[PATCH v2 1/6] ima: Set file->f_mode instead of file->f_flags in ima_calc_file_hash()

[PATCH v2 1/6] ima: Set file->f_mode instead of file->f_flags in ima_calc_file_hash()

[Roberto Sassu]

Commit a408e4a86b36 ("ima: open a new file instance if no read
permissions") tries to create a new file descriptor to calculate a file
digest if the file has not been opened with O_RDONLY flag. However, if a
new file descriptor cannot be obtained, it sets the FMODE_READ flag to
file->f_flags instead of file->f_mode.

This patch fixes this issue by replacing f_flags with f_mode as it was
before that commit.

Changelog

v1:
- fix comment for f_mode change (suggested by Mimi)
- rename modified_flags variable to modified_mode (suggested by Mimi)

Cc: stable@vger.kernel.org # 4.20.x
Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_crypto.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
index 5201f5ec2ce4..f3a7f4eb1fc1 100644
--- a/security/integrity/ima/ima_crypto.c
+++ b/security/integrity/ima/ima_crypto.c
@@ -537,7 +537,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 	loff_t i_size;
 	int rc;
 	struct file *f = file;
-	bool new_file_instance = false, modified_flags = false;
+	bool new_file_instance = false, modified_mode = false;
 
 	/*
 	 * For consistency, fail file's opened with the O_DIRECT flag on
@@ -557,13 +557,13 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 		f = dentry_open(&file->f_path, flags, file->f_cred);
 		if (IS_ERR(f)) {
 			/*
-			 * Cannot open the file again, lets modify f_flags
+			 * Cannot open the file again, lets modify f_mode
 			 * of original and continue
 			 */
 			pr_info_ratelimited("Unable to reopen file for reading.\n");
 			f = file;
-			f->f_flags |= FMODE_READ;
-			modified_flags = true;
+			f->f_mode |= FMODE_READ;
+			modified_mode = true;
 		} else {
 			new_file_instance = true;
 		}
@@ -581,8 +581,8 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
 out:
 	if (new_file_instance)
 		fput(f);
-	else if (modified_flags)
-		f->f_flags &= ~FMODE_READ;
+	else if (modified_mode)
+		f->f_mode &= ~FMODE_READ;
 	return rc;
 }
 
-- 
2.17.1

[PATCH v2 2/6] evm: Check also if *tfm is an error pointer in init_desc()

[Roberto Sassu]

This patch avoids a kernel panic due to accessing an error pointer set by
crypto_alloc_shash(). It occurs especially when there are many files that
require an unsupported algorithm, as it would increase the likelihood of
the following race condition:

Task A: *tfm = crypto_alloc_shash() <= error pointer
Task B: if (*tfm == NULL) <= *tfm is not NULL, use it
Task B: rc = crypto_shash_init(desc) <= panic
Task A: *tfm = NULL

This patch uses the IS_ERR_OR_NULL macro to determine whether or not a new
crypto context must be created.

Cc: stable@vger.kernel.org
Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
Co-developed-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/evm/evm_crypto.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/security/integrity/evm/evm_crypto.c b/security/integrity/evm/evm_crypto.c
index 35682852ddea..77ad1e5a93e4 100644
--- a/security/integrity/evm/evm_crypto.c
+++ b/security/integrity/evm/evm_crypto.c
@@ -91,7 +91,7 @@ static struct shash_desc *init_desc(char type, uint8_t hash_algo)
 		algo = hash_algo_name[hash_algo];
 	}
 
-	if (*tfm == NULL) {
+	if (IS_ERR_OR_NULL(*tfm)) {
 		mutex_lock(&mutex);
 		if (*tfm)
 			goto out;
-- 
2.17.1

[PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[Roberto Sassu]

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(-)

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)		\
-- 
2.17.1

RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[David Laight]

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)

RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[Roberto Sassu]

> -----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)

RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[David Laight]

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)

RE: [PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[Roberto Sassu]

> -----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)

[RESEND][PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[Roberto Sassu]

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 first two bytes of the digest with the number of slots.
Also reduce the depth of the hash table by doubling the number of slots.

Changelog

v2: directly access the first two bytes of the digest to avoid memory
    access issues on big endian systems (suggested by David Laight)

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 | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index 467dfdbea25c..02796473238b 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,10 @@ 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);
+	/* there is no point in taking a hash of part of a digest */
+	return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
 }
 
 #define __ima_hooks(hook)		\
-- 
2.17.1

RE: [RESEND][PATCH v2 3/6] ima: Fix ima digest hash table key calculation

[David Laight]

From: Roberto Sassu
> Sent: 28 April 2020 08:30
> 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 first two bytes of the digest with the number of slots.
> Also reduce the depth of the hash table by doubling the number of slots.
> 
> Changelog
> 
> v2: directly access the first two bytes of the digest to avoid memory
>     access issues on big endian systems (suggested by David Laight)
> 
> 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>

Acked-by: David.Laight@aculab.com

> ---
>  security/integrity/ima/ima.h | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index 467dfdbea25c..02796473238b 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,10 @@ 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);
> +	/* there is no point in taking a hash of part of a digest */
> +	return (digest[0] | digest[1] << 8) % IMA_MEASURE_HTABLE_SIZE;
>  }
> 
>  #define __ima_hooks(hook)		\
> --
> 2.17.1

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

[PATCH v2 4/6] ima: Remove redundant policy rule set in add_rules()

[Roberto Sassu]

From: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

Function ima_appraise_flag() returns the flag to be set in
temp_ima_appraise depending on the hook identifier passed as an argument.
It is not necessary to set the flag again for the POLICY_CHECK hook.

Signed-off-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>
---
 security/integrity/ima/ima_policy.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index c334e0dc6083..ea9b991f0232 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -643,11 +643,8 @@ static void add_rules(struct ima_rule_entry *entries, int count,
 
 			list_add_tail(&entry->list, &ima_policy_rules);
 		}
-		if (entries[i].action == APPRAISE) {
+		if (entries[i].action == APPRAISE)
 			temp_ima_appraise |= ima_appraise_flag(entries[i].func);
-			if (entries[i].func == POLICY_CHECK)
-				temp_ima_appraise |= IMA_APPRAISE_POLICY;
-		}
 	}
 }
 
-- 
2.17.1

[PATCH v2 5/6] ima: Set again build_ima_appraise variable

[Roberto Sassu]

From: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

After adding the new add_rule() function in commit c52657d93b05
("ima: refactor ima_init_policy()"), all appraisal flags are added to the
temp_ima_appraise variable. Revert to the previous behavior instead of
removing build_ima_appraise, to benefit from the protection offered by
__ro_after_init.

The mentioned commit introduced a bug, as it makes all the flags
modifiable, while build_ima_appraise flags can be protected with
__ro_after_init.

Changelog

v1:
- set build_ima_appraise instead of removing it (suggested by Mimi)

Cc: stable@vger.kernel.org # 5.0.x
Fixes: c52657d93b05 ("ima: refactor ima_init_policy()")
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_policy.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index ea9b991f0232..ef7f68cc935e 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -643,8 +643,14 @@ static void add_rules(struct ima_rule_entry *entries, int count,
 
 			list_add_tail(&entry->list, &ima_policy_rules);
 		}
-		if (entries[i].action == APPRAISE)
-			temp_ima_appraise |= ima_appraise_flag(entries[i].func);
+		if (entries[i].action == APPRAISE) {
+			if (entries != build_appraise_rules)
+				temp_ima_appraise |=
+					ima_appraise_flag(entries[i].func);
+			else
+				build_ima_appraise |=
+					ima_appraise_flag(entries[i].func);
+		}
 	}
 }
 
-- 
2.17.1

[PATCH v2 6/6] ima: Fix return value of ima_write_policy()

[Roberto Sassu]

This patch fixes the return value of ima_write_policy() when a new policy
is directly passed to IMA and the current policy requires appraisal of the
file containing the policy. Currently, if appraisal is not in ENFORCE mode,
ima_write_policy() returns 0 and leads user space applications to an
endless loop. Fix this issue by denying the operation regardless of the
appraisal mode.

Changelog

v1:
- deny the operation in all cases (suggested by Mimi, Krzysztof)

Cc: stable@vger.kernel.org # 4.10.x
Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
---
 security/integrity/ima/ima_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 8b030a1c5e0d..e3fcad871861 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -338,8 +338,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
 				    "policy_update", "signed policy required",
 				    1, 0);
-		if (ima_appraise & IMA_APPRAISE_ENFORCE)
-			result = -EACCES;
+		result = -EACCES;
 	} else {
 		result = ima_parse_add_rule(data);
 	}
-- 
2.17.1

Re: [PATCH v2 6/6] ima: Fix return value of ima_write_policy()

[Mimi Zohar]

Hi Roberto,

On Mon, 2020-04-27 at 12:31 +0200, Roberto Sassu wrote:
> This patch fixes the return value of ima_write_policy() when a new policy
> is directly passed to IMA and the current policy requires appraisal of the
> file containing the policy. Currently, if appraisal is not in ENFORCE mode,
> ima_write_policy() returns 0 and leads user space applications to an
> endless loop. Fix this issue by denying the operation regardless of the
> appraisal mode.
> 
> Changelog
> 
> v1:
> - deny the operation in all cases (suggested by Mimi, Krzysztof)

Relatively recently, people have moved away from including the
"Changelog" in the upstream commit. (I'm removing them now.)  

> 
> Cc: stable@vger.kernel.org # 4.10.x
> Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>

Without the Changelog, the only way of acknowledging people's
contributions is by including their tags.  Krzysztof, did you want to
add your "Reviewed-by" tag?

> ---

People have started putting the Changelog or any comments immediately
below the separator "---" here.

thanks,

Mimi

>  security/integrity/ima/ima_fs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 8b030a1c5e0d..e3fcad871861 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -338,8 +338,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
>  				    "policy_update", "signed policy required",
>  				    1, 0);
> -		if (ima_appraise & IMA_APPRAISE_ENFORCE)
> -			result = -EACCES;
> +		result = -EACCES;
>  	} else {
>  		result = ima_parse_add_rule(data);
>  	}

RE: [PATCH v2 6/6] ima: Fix return value of ima_write_policy()

[Krzysztof Struczynski]

Hi Mimi,

> -----Original Message-----
> From: Mimi Zohar [mailto:zohar@linux.ibm.com]
> Sent: Tuesday, April 28, 2020 7:47 PM
> To: Roberto Sassu <roberto.sassu@huawei.com>; Krzysztof Struczynski
> <krzysztof.struczynski@huawei.com>
> 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 6/6] ima: Fix return value of ima_write_policy()
> 
> Hi Roberto,
> 
> On Mon, 2020-04-27 at 12:31 +0200, Roberto Sassu wrote:
> > This patch fixes the return value of ima_write_policy() when a new
> > policy is directly passed to IMA and the current policy requires
> > appraisal of the file containing the policy. Currently, if appraisal
> > is not in ENFORCE mode,
> > ima_write_policy() returns 0 and leads user space applications to an
> > endless loop. Fix this issue by denying the operation regardless of
> > the appraisal mode.
> >
> > Changelog
> >
> > v1:
> > - deny the operation in all cases (suggested by Mimi, Krzysztof)
> 
> Relatively recently, people have moved away from including the "Changelog"
> in the upstream commit. (I'm removing them now.)
> 
> >
> > Cc: stable@vger.kernel.org # 4.10.x
> > Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy
> > itself")
> > Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> 
> Without the Changelog, the only way of acknowledging people's contributions
> is by including their tags.  Krzysztof, did you want to add your "Reviewed-by"
> tag?

Please add:
Reviewed-by: Krzysztof Struczynski <krzysztof.struczynski@huawei.com>

Thanks,
Krzysztof

> 
> > ---
> 
> People have started putting the Changelog or any comments immediately
> below the separator "---" here.
> 
> thanks,
> 
> Mimi
> 
> >  security/integrity/ima/ima_fs.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c index 8b030a1c5e0d..e3fcad871861
> > 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -338,8 +338,7 @@ static ssize_t ima_write_policy(struct file *file, const
> char __user *buf,
> >  		integrity_audit_msg(AUDIT_INTEGRITY_STATUS, NULL, NULL,
> >  				    "policy_update", "signed policy required",
> >  				    1, 0);
> > -		if (ima_appraise & IMA_APPRAISE_ENFORCE)
> > -			result = -EACCES;
> > +		result = -EACCES;
> >  	} else {
> >  		result = ima_parse_add_rule(data);
> >  	}

Re: [PATCH v2 1/6] ima: Set file->f_mode instead of file->f_flags in ima_calc_file_hash()

[Goldwyn Rodrigues]

On 12:28 27/04, Roberto Sassu wrote:
> Commit a408e4a86b36 ("ima: open a new file instance if no read
> permissions") tries to create a new file descriptor to calculate a file
> digest if the file has not been opened with O_RDONLY flag. However, if a
> new file descriptor cannot be obtained, it sets the FMODE_READ flag to
> file->f_flags instead of file->f_mode.
> 
> This patch fixes this issue by replacing f_flags with f_mode as it was
> before that commit.

Thanks for fixing this.

Reviewed-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> 
> Changelog
> 
> v1:
> - fix comment for f_mode change (suggested by Mimi)
> - rename modified_flags variable to modified_mode (suggested by Mimi)
> 
> Cc: stable@vger.kernel.org # 4.20.x
> Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
> Signed-off-by: Roberto Sassu <roberto.sassu@huawei.com>
> ---
>  security/integrity/ima/ima_crypto.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/security/integrity/ima/ima_crypto.c b/security/integrity/ima/ima_crypto.c
> index 5201f5ec2ce4..f3a7f4eb1fc1 100644
> --- a/security/integrity/ima/ima_crypto.c
> +++ b/security/integrity/ima/ima_crypto.c
> @@ -537,7 +537,7 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  	loff_t i_size;
>  	int rc;
>  	struct file *f = file;
> -	bool new_file_instance = false, modified_flags = false;
> +	bool new_file_instance = false, modified_mode = false;
>  
>  	/*
>  	 * For consistency, fail file's opened with the O_DIRECT flag on
> @@ -557,13 +557,13 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  		f = dentry_open(&file->f_path, flags, file->f_cred);
>  		if (IS_ERR(f)) {
>  			/*
> -			 * Cannot open the file again, lets modify f_flags
> +			 * Cannot open the file again, lets modify f_mode
>  			 * of original and continue
>  			 */
>  			pr_info_ratelimited("Unable to reopen file for reading.\n");
>  			f = file;
> -			f->f_flags |= FMODE_READ;
> -			modified_flags = true;
> +			f->f_mode |= FMODE_READ;
> +			modified_mode = true;
>  		} else {
>  			new_file_instance = true;
>  		}
> @@ -581,8 +581,8 @@ int ima_calc_file_hash(struct file *file, struct ima_digest_data *hash)
>  out:
>  	if (new_file_instance)
>  		fput(f);
> -	else if (modified_flags)
> -		f->f_flags &= ~FMODE_READ;
> +	else if (modified_mode)
> +		f->f_mode &= ~FMODE_READ;
>  	return rc;
>  }
>  
> -- 
> 2.17.1
> 

-- 
Goldwyn