[v2,3/6] ima: Fix ima digest hash table key calculation
diff mbox series

Message ID 20200427102900.18887-3-roberto.sassu@huawei.com
State New
Headers show
Series
  • [v2,1/6] ima: Set file->f_mode instead of file->f_flags in ima_calc_file_hash()
Related show

Commit Message

Roberto Sassu April 27, 2020, 10:28 a.m. UTC
From: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

Function hash_long() accepts unsigned long, while currently only one byte
is passed from ima_hash_key(), which calculates a key for ima_htable.

Given that hashing the digest does not give clear benefits compared to
using the digest itself, remove hash_long() and return the modulus
calculated on the beginning of the digest with the number of slots. Also
reduce the depth of the hash table by doubling the number of slots.

Cc: stable@vger.kernel.org
Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
---
 security/integrity/ima/ima.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

David Laight April 27, 2020, 11 a.m. UTC | #1
From: Roberto Sassu
> Sent: 27 April 2020 11:29
> Function hash_long() accepts unsigned long, while currently only one byte
> is passed from ima_hash_key(), which calculates a key for ima_htable.
> 
> Given that hashing the digest does not give clear benefits compared to
> using the digest itself, remove hash_long() and return the modulus
> calculated on the beginning of the digest with the number of slots. Also
> reduce the depth of the hash table by doubling the number of slots.
> 
> Cc: stable@vger.kernel.org
> Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
> Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> ---
>  security/integrity/ima/ima.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 467dfdbea25c..6ee458cf124a 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -36,7 +36,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
>  #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
>  #define IMA_EVENT_NAME_LEN_MAX	255
> 
> -#define IMA_HASH_BITS 9
> +#define IMA_HASH_BITS 10
>  #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
> 
>  #define IMA_TEMPLATE_FIELD_ID_MAX_LEN	16
> @@ -179,9 +179,9 @@ struct ima_h_table {
>  };
>  extern struct ima_h_table ima_htable;
> 
> -static inline unsigned long ima_hash_key(u8 *digest)
> +static inline unsigned int ima_hash_key(u8 *digest)
>  {
> -	return hash_long(*digest, IMA_HASH_BITS);
> +	return (*(unsigned int *)digest % IMA_MEASURE_HTABLE_SIZE);

That almost certainly isn't right.
It falls foul of the *(integer_type *)ptr being almost always wrong.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Roberto Sassu April 27, 2020, 12:50 p.m. UTC | #2
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Monday, April 27, 2020 1:00 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> rgoldwyn@suse.de
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>; stable@vger.kernel.org
> Subject: RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation
> 
> From: Roberto Sassu
> > Sent: 27 April 2020 11:29
> > Function hash_long() accepts unsigned long, while currently only one byte
> > is passed from ima_hash_key(), which calculates a key for ima_htable.
> >
> > Given that hashing the digest does not give clear benefits compared to
> > using the digest itself, remove hash_long() and return the modulus
> > calculated on the beginning of the digest with the number of slots. Also
> > reduce the depth of the hash table by doubling the number of slots.
> >
> > Cc: stable@vger.kernel.org
> > Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
> > Co-developed-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
> > ---
> >  security/integrity/ima/ima.h | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 467dfdbea25c..6ee458cf124a 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -36,7 +36,7 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
> >  #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
> >  #define IMA_EVENT_NAME_LEN_MAX	255
> >
> > -#define IMA_HASH_BITS 9
> > +#define IMA_HASH_BITS 10
> >  #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
> >
> >  #define IMA_TEMPLATE_FIELD_ID_MAX_LEN	16
> > @@ -179,9 +179,9 @@ struct ima_h_table {
> >  };
> >  extern struct ima_h_table ima_htable;
> >
> > -static inline unsigned long ima_hash_key(u8 *digest)
> > +static inline unsigned int ima_hash_key(u8 *digest)
> >  {
> > -	return hash_long(*digest, IMA_HASH_BITS);
> > +	return (*(unsigned int *)digest % IMA_MEASURE_HTABLE_SIZE);
> 
> That almost certainly isn't right.
> It falls foul of the *(integer_type *)ptr being almost always wrong.

I didn't find the problem. Can you please explain?

Thanks

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)
David Laight April 27, 2020, 2:28 p.m. UTC | #3
From: Roberto Sassu
> Sent: 27 April 2020 13:51
...
> > > -static inline unsigned long ima_hash_key(u8 *digest)
> > > +static inline unsigned int ima_hash_key(u8 *digest)
> > >  {
> > > -	return hash_long(*digest, IMA_HASH_BITS);
> > > +	return (*(unsigned int *)digest % IMA_MEASURE_HTABLE_SIZE);
> >
> > That almost certainly isn't right.
> > It falls foul of the *(integer_type *)ptr being almost always wrong.
> 
> I didn't find the problem. Can you please explain?

The general problem with *(int_type *)ptr is that it does completely
the wrong thing if 'ptr' is the address of a larger integer type on
a big-endian system.
You may also get a misaligned access trap.

In this case I guess that digest is actually u8[SHA1_DIGEST_SIZE].
Maybe what you should return is:
	(digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
and comment that there is no point taking a hash of part of
a SHA1 digest.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
Roberto Sassu April 28, 2020, 7:19 a.m. UTC | #4
> -----Original Message-----
> From: David Laight [mailto:David.Laight@ACULAB.COM]
> Sent: Monday, April 27, 2020 4:28 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>; zohar@linux.ibm.com;
> rgoldwyn@suse.de
> Cc: linux-integrity@vger.kernel.org; linux-security-module@vger.kernel.org;
> linux-kernel@vger.kernel.org; Silviu Vlasceanu
> <Silviu.Vlasceanu@huawei.com>; Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>; stable@vger.kernel.org
> Subject: RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation
> 
> From: Roberto Sassu
> > Sent: 27 April 2020 13:51
> ...
> > > > -static inline unsigned long ima_hash_key(u8 *digest)
> > > > +static inline unsigned int ima_hash_key(u8 *digest)
> > > >  {
> > > > -	return hash_long(*digest, IMA_HASH_BITS);
> > > > +	return (*(unsigned int *)digest % IMA_MEASURE_HTABLE_SIZE);
> > >
> > > That almost certainly isn't right.
> > > It falls foul of the *(integer_type *)ptr being almost always wrong.
> >
> > I didn't find the problem. Can you please explain?
> 
> The general problem with *(int_type *)ptr is that it does completely
> the wrong thing if 'ptr' is the address of a larger integer type on
> a big-endian system.
> You may also get a misaligned access trap.
> 
> In this case I guess that digest is actually u8[SHA1_DIGEST_SIZE].
> Maybe what you should return is:
> 	(digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
> and comment that there is no point taking a hash of part of
> a SHA1 digest.

Ok, thanks.

Roberto

HUAWEI TECHNOLOGIES Duesseldorf GmbH, HRB 56063
Managing Director: Li Peng, Li Jian, Shi Yanli


> 	David
> 
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes,
> MK1 1PT, UK
> Registration No: 1397386 (Wales)

Patch
diff mbox series

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 467dfdbea25c..6ee458cf124a 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -36,7 +36,7 @@  enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8 };
 #define IMA_DIGEST_SIZE		SHA1_DIGEST_SIZE
 #define IMA_EVENT_NAME_LEN_MAX	255
 
-#define IMA_HASH_BITS 9
+#define IMA_HASH_BITS 10
 #define IMA_MEASURE_HTABLE_SIZE (1 << IMA_HASH_BITS)
 
 #define IMA_TEMPLATE_FIELD_ID_MAX_LEN	16
@@ -179,9 +179,9 @@  struct ima_h_table {
 };
 extern struct ima_h_table ima_htable;
 
-static inline unsigned long ima_hash_key(u8 *digest)
+static inline unsigned int ima_hash_key(u8 *digest)
 {
-	return hash_long(*digest, IMA_HASH_BITS);
+	return (*(unsigned int *)digest % IMA_MEASURE_HTABLE_SIZE);
 }
 
 #define __ima_hooks(hook)		\