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: [email protected] # 4.20.x
Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
Signed-off-by: Roberto Sassu <[email protected]>
---
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
From: Krzysztof Struczynski <[email protected]>
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 <[email protected]>
---
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
From: Krzysztof Struczynski <[email protected]>
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: [email protected]
Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
Co-developed-by: Roberto Sassu <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
Signed-off-by: Krzysztof Struczynski <[email protected]>
---
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
From: Krzysztof Struczynski <[email protected]>
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: [email protected] # 5.0.x
Fixes: c52657d93b05 ("ima: refactor ima_init_policy()")
Co-developed-by: Roberto Sassu <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
Signed-off-by: Krzysztof Struczynski <[email protected]>
---
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
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: [email protected]
Fixes: d46eb3699502b ("evm: crypto hash replaced by shash")
Co-developed-by: Krzysztof Struczynski <[email protected]>
Signed-off-by: Krzysztof Struczynski <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
---
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
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: [email protected] # 4.10.x
Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
Signed-off-by: Roberto Sassu <[email protected]>
---
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
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: [email protected]
> Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
> Co-developed-by: Roberto Sassu <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
> Signed-off-by: Krzysztof Struczynski <[email protected]>
> ---
> 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)
> -----Original Message-----
> From: David Laight [mailto:[email protected]]
> Sent: Monday, April 27, 2020 1:00 PM
> To: Roberto Sassu <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Silviu Vlasceanu
> <[email protected]>; Krzysztof Struczynski
> <[email protected]>; [email protected]
> 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: [email protected]
> > Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
> > Co-developed-by: Roberto Sassu <[email protected]>
> > Signed-off-by: Roberto Sassu <[email protected]>
> > Signed-off-by: Krzysztof Struczynski <[email protected]>
> > ---
> > 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)
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 <[email protected]>
>
> Changelog
>
> v1:
> - fix comment for f_mode change (suggested by Mimi)
> - rename modified_flags variable to modified_mode (suggested by Mimi)
>
> Cc: [email protected] # 4.20.x
> Fixes: a408e4a86b36 ("ima: open a new file instance if no read permissions")
> Signed-off-by: Roberto Sassu <[email protected]>
> ---
> 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
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)
> -----Original Message-----
> From: David Laight [mailto:[email protected]]
> Sent: Monday, April 27, 2020 4:28 PM
> To: Roberto Sassu <[email protected]>; [email protected];
> [email protected]
> Cc: [email protected]; [email protected];
> [email protected]; Silviu Vlasceanu
> <[email protected]>; Krzysztof Struczynski
> <[email protected]>; [email protected]
> 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)
From: Krzysztof Struczynski <[email protected]>
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: [email protected]
Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
Co-developed-by: Roberto Sassu <[email protected]>
Signed-off-by: Roberto Sassu <[email protected]>
Signed-off-by: Krzysztof Struczynski <[email protected]>
---
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
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: [email protected] # 4.10.x
> Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy itself")
> Signed-off-by: Roberto Sassu <[email protected]>
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);
> }
Hi Mimi,
> -----Original Message-----
> From: Mimi Zohar [mailto:[email protected]]
> Sent: Tuesday, April 28, 2020 7:47 PM
> To: Roberto Sassu <[email protected]>; Krzysztof Struczynski
> <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; Silviu Vlasceanu
> <[email protected]>; Krzysztof Struczynski
> <[email protected]>; [email protected]
> 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: [email protected] # 4.10.x
> > Fixes: 19f8a84713edc ("ima: measure and appraise the IMA policy
> > itself")
> > Signed-off-by: Roberto Sassu <[email protected]>
>
> 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 <[email protected]>
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);
> > }
From: Roberto Sassu
> Sent: 28 April 2020 08:30
> From: Krzysztof Struczynski <[email protected]>
>
> 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: [email protected]
> Fixes: 3323eec921ef ("integrity: IMA as an integrity service provider")
> Co-developed-by: Roberto Sassu <[email protected]>
> Signed-off-by: Roberto Sassu <[email protected]>
> Signed-off-by: Krzysztof Struczynski <[email protected]>
Acked-by: [email protected]
> ---
> 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)