[PATCH] IMA: add an option to restrict the accepted hash algorithms

[PATCH] IMA: add an option to restrict the accepted hash algorithms

[THOBY Simon]

Sorry it took me some time before I could write a patch (in addition it's my
first kernel contribution so I'm still learning how things should be done, like
fighting Outlook on the web, giving up on it and moving to Mozilla Thunderbird).

This patch is a follow up on the discussion at
https://lore.kernel.org/linux-integrity/10dde047d76b447f32ca91356599be679b8a76e5.camel@linux.ibm.com/t/#m0f8127c6982ef94aa42f5cc13ea83b9f9000917e.


This patch was built on top of linux-5.14.0-rc1, and I successfully tested the patch
in the following configurations:
- disabled: no visible impact (as expected)
- with ima_allowed_hashes=sha256,sha512:
	- alone: blocks both execution and xattr writes
	  (tested with `emvctl ima_hash -a md5 <my binary>`)
	- with ima_appraise=log: audit logs but still permits access
	- with ima_appraise=fix: audit logs but still permits access, however it
	  doesn't fix the hash (so maybe I should do something about it, because
	  right now it's basically the same as ima_appraise=log w.r.t. hash
	  algorithms ?)
	- with ima_accept_any_hash: permits access, no warnings whatsoever
Do you want ideas of other configurations that I could test?

I would also like to point out that I'm more than open to suggestions for
changing the names of the parameters (`ima_allowed_hashes` and
`ima_accept_any_hash`) and the "cause" in the audit log (currently 
forbidden-hash-algorithm"), because as you know, "naming things is hard".


IMA: add an option to restrict the accepted hash algorithms

Adds two command-line parameters to limit the hash algorithms
allowed for the security.ima xattr. This gives users the
ability to ensure their systems will not accept weak hashes,
and potentially increase users trust in their IMA configuration,
because they can ensure only strong collision-resistant hashes
are employed and files generated otherwise will not be accepted.

The main point is to safeguard users from mislabelling their files
when using userland utilities to update their files, e.g. evmctl
(`evmctl ima_hash` defaults to sha1). Another possible target
would be people that have deployed IMA years ago, possibly using
algorithms that was then deemed sufficiently collision-resistant,
but that proved to be weak with the passage of time (note that this
could also happen in the future with algorithms considered safe today).
This patch also provides a migration path for users.

The first parameter supplies a list of allowed algorithms to
the kernel, restricting appraisal to files hashed with "strong"
hash algorithms (by default IMA will keep accepting any hash
algorithm, as enabling such a feature by default would both be
backward-incompatible and very probably break real systems).
The second parameter is an escape hatch that adds a weaker form
of backward-compatibility: when activated, IMA apparaisal will
keep working on files hashed with an algorithm not present in
the list, but updates to these files or new writes to the security.ima
xattr will enforce the selected hash algorithms. This may be useful
to perform an online migration from one algorithm to another.


Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
---
 .../admin-guide/kernel-parameters.txt         | 42 +++++++++
 security/integrity/ima/ima.h                  |  6 +-
 security/integrity/ima/ima_appraise.c         | 22 +++++
 security/integrity/ima/ima_main.c             | 88 ++++++++++++++++++-
 4 files changed, 156 insertions(+), 2 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index bdb22006f713..af7567b3a44e 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -1817,6 +1817,48 @@
 			The list of supported hash algorithms is defined
 			in crypto/hash_info.h.
 
+	ima_allowed_hashes= [IMA]
+			Format: comma-separated list of { md5 | sha1 | rmd160
+				| sha256 | sha384 | sha512 | ... }
+
+			When using IMA in hashing mode, this attribute specifies the
+			list of hash algorithms that are allowed in the security.ima
+			xattr. Files hashed with an algorithm not present in that
+			list will not pass appraisal, and writes of invalid xattrs
+			are disallowed too.
+
+			Use this option to ensure only strong and collision-resistant
+			hashes are used and accepted by the kernel, instead of the
+			default that will happily appraise files hashed with any of
+			the numerous hash algorithms the kenrel supports.
+
+			See also "ima_accept_any_hash" to only restrict writes.
+			The list of supported hash algorithms is defined
+			in crypto/hash_info.h.
+
+	ima_accept_any_hash [IMA]
+
+			Accept for appraisal files with an IMA hash not allowed
+			in "ima_allowed_hashes", to allow iterative migrations
+			of your files to stronger algorithms.
+
+			When using IMA in hashing mode with "ima_allowed_hashes"
+			selected, the kernel restricts the hash algorithms
+			considered valid, and prevent both writes of files with
+			invalid xattrs and execution/read/... (depending on
+			the IMA policy) of such files.
+
+			This option changes that behavior to be backward compatible:
+			the kernel accept for appraisal files hashed with algorithms
+			not present in the list, but still enforces the algorithms
+			on write.
+			This may be useful for migrating to a new hash algorithm:
+			first changing ima_allowed_hashed and adding the ima_accept_any_hash
+			parameter to keep accepting the old files, rebooting and
+			relabelling all the files on the system, and finally
+			rebooting again without the ima_accept_any_hash parameter
+			to enforce the new policy.
+
 	ima_policy=	[IMA]
 			The builtin policies to load during IMA setup.
 			Format: "tcb | appraise_tcb | secure_boot |
diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index f0e448ed1f9f..ef173fcb7112 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -21,6 +21,7 @@
 #include <linux/tpm.h>
 #include <linux/audit.h>
 #include <crypto/hash_info.h>
+#include <stdbool.h>
 
 #include "../integrity.h"
 
@@ -47,7 +48,9 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 extern int ima_policy_flag;
 
 /* set during initialization */
-extern int ima_hash_algo;
+extern int ima_hash_algo __ro_after_init;
+extern unsigned int ima_allowed_hashes __ro_after_init;
+extern bool ima_accept_any_hash __ro_after_init;
 extern int ima_sha1_idx __ro_after_init;
 extern int ima_hash_algo_idx __ro_after_init;
 extern int ima_extra_slots __ro_after_init;
@@ -164,6 +167,7 @@ void ima_init_template_list(void);
 int __init ima_init_digests(void);
 int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
 			  void *lsm_data);
+int ima_validate_hash_algorithm(enum hash_algo hash_alg, bool xattr_update);
 
 /*
  * used to protect h_table and sha_table
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index ef9dcfce45d4..ecbfd644dca8 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -592,6 +592,28 @@ int ima_inode_setxattr(struct dentry *dentry, const char *xattr_name,
 		digsig = (xvalue->type == EVM_XATTR_PORTABLE_DIGSIG);
 	}
 	if (result == 1 || evm_revalidate_status(xattr_name)) {
+		enum hash_algo hash_alg =
+			ima_get_hash_algo((struct evm_ima_xattr_data *)xattr_value,
+					  xattr_value_len);
+
+		/* Ensure the user-supplied xattr value uses a whitelisted hash algorithm */
+		int rc = ima_validate_hash_algorithm(hash_alg, true);
+		if (rc != 0) {
+			char *path = NULL;
+			char *pathbuf = kmalloc(PATH_MAX, GFP_KERNEL);
+			/* no memory available ? no file path for you */
+			if (pathbuf)
+				path = dentry_path(dentry, pathbuf, PATH_MAX);
+
+			integrity_audit_msg(AUDIT_INTEGRITY_DATA, d_inode(dentry),
+				path, "collect_data", "forbidden-hash-algorithm", rc, 0);
+
+			if (pathbuf)
+				kfree(pathbuf);
+
+			return rc;
+		}
+
 		ima_reset_appraise_flags(d_backing_inode(dentry), digsig);
 		if (result == 1)
 			result = 0;
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 287b90509006..4206f2459bc1 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -38,6 +38,17 @@ int ima_appraise;
 int ima_hash_algo = HASH_ALGO_SHA1;
 static int hash_setup_done;
 
+unsigned int ima_allowed_hashes;
+static_assert(
+	8 * sizeof(ima_allowed_hashes) >= HASH_ALGO__LAST,
+	"The bitfield ima_allowed_hashes is too small to contain all the supported hash algorithms");
+static char *ima_allowed_hashes_cmdline __initdata;
+core_param(ima_allowed_hashes, ima_allowed_hashes_cmdline, charp, 0);
+
+bool ima_accept_any_hash;
+core_param(ima_accept_any_hash, ima_accept_any_hash, bool, 0);
+
+
 static struct notifier_block ima_lsm_policy_notifier = {
 	.notifier_call = ima_lsm_policy_change,
 };
@@ -76,6 +87,46 @@ static int __init hash_setup(char *str)
 }
 __setup("ima_hash=", hash_setup);
 
+static void __init hash_restrictions_setup(void)
+{
+	char *str_copy, *iter;
+
+	if (!ima_allowed_hashes_cmdline)
+		return;
+
+	pr_info("restricting allowed hash algorithms to %s", ima_allowed_hashes_cmdline);
+
+	/* copy the string to perform strsep() on it */
+	str_copy = kmalloc(strlen(ima_allowed_hashes_cmdline) + 1, GFP_KERNEL);
+	if (!str_copy) {
+		pr_err("couldn't allocate memory for parsing the list of allowed hashes");
+		return;
+	}
+
+	memcpy(str_copy, ima_allowed_hashes_cmdline, strlen(ima_allowed_hashes_cmdline) + 1);
+
+	iter = str_copy;
+	do {
+		int idx;
+		char *token = strsep(&iter, ",");
+
+		if (token == NULL)
+			break;
+
+		idx = match_string(hash_algo_name, HASH_ALGO__LAST, token);
+		if (idx < 0) {
+			pr_err("invalid hash algorithm \"%s\", ignoring",
+			       token);
+			continue;
+		}
+
+		/* Add the hash algorithm to the 'allowed' bitfield */
+		ima_allowed_hashes |= (1U << idx);
+	} while (iter != NULL);
+
+	kfree(str_copy);
+}
+
 /* Prevent mmap'ing a file execute that is already mmap'ed write */
 static int mmap_violation_check(enum ima_hooks func, struct file *file,
 				char **pathbuf, const char **pathname,
@@ -172,6 +223,29 @@ static void ima_check_last_writer(struct integrity_iint_cache *iint,
 	mutex_unlock(&iint->mutex);
 }
 
+/**
+ * ima_validate_hash_algorithm
+ * @hash_alg: the requested hash algorithm
+ * @attr_update: true if the user is trying to write the security.ima
+ * xattr with 'hash_alg' format. False means we are performing appraisal
+ * against a file that possesses a xattr hashed with 'hash_alg'.
+ *
+ * Ensure that the digest was generated using an allowed algorithm
+ */
+int ima_validate_hash_algorithm(enum hash_algo hash_alg, bool xattr_update)
+{
+	/* when reading/executing a file in "permissive" mode, accept any hash */
+	if (ima_accept_any_hash && !xattr_update)
+		return 0;
+
+	if (ima_allowed_hashes != 0 &&
+	    unlikely(!(ima_allowed_hashes & (1U << hash_alg)))) {
+		return -EACCES;
+	}
+
+	return 0;
+}
+
 /**
  * ima_file_free - called on __fput()
  * @file: pointer to file structure being freed
@@ -327,11 +401,22 @@ static int process_measurement(struct file *file, const struct cred *cred,
 
 	hash_algo = ima_get_hash_algo(xattr_value, xattr_len);
 
+	rc = ima_validate_hash_algorithm(hash_algo, false);
+	if (rc != 0) {
+		if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
+			pathname = ima_d_path(&file->f_path, &pathbuf, filename);
+
+		integrity_audit_msg(AUDIT_INTEGRITY_DATA, file_inode(file),
+			pathname, "collect_data", "forbidden-hash-algorithm", rc, 0);
+
+		goto out_locked;
+	}
+
 	rc = ima_collect_measurement(iint, file, buf, size, hash_algo, modsig);
 	if (rc != 0 && rc != -EBADF && rc != -EINVAL)
 		goto out_locked;
 
-	if (!pathbuf)	/* ima_rdwr_violation possibly pre-fetched */
+	if (!pathbuf)	/* ima_rdwr_violation or hash_algs checks above possibly pre-fetched */
 		pathname = ima_d_path(&file->f_path, &pathbuf, filename);
 
 	if (action & IMA_MEASURE)
@@ -993,6 +1078,7 @@ static int __init init_ima(void)
 	ima_appraise_parse_cmdline();
 	ima_init_template_list();
 	hash_setup(CONFIG_IMA_DEFAULT_HASH);
+	hash_restrictions_setup();
 	error = ima_init();
 
 	if (error && strcmp(hash_algo_name[ima_hash_algo],
-- 
2.31.1

Re: [PATCH] IMA: add an option to restrict the accepted hash algorithms

[Mimi Zohar]

Hi Simon,

On Tue, 2021-07-13 at 14:48 +0000, THOBY Simon wrote:
> Sorry it took me some time before I could write a patch (in addition it's my
> first kernel contribution so I'm still learning how things should be done, like
> fighting Outlook on the web, giving up on it and moving to Mozilla Thunderbird).

Thanks!

> 
> This patch is a follow up on the discussion at
> https://lore.kernel.org/linux-integrity/10dde047d76b447f32ca91356599be679b8a76e5.camel@linux.ibm.com/t/#m0f8127c6982ef94aa42f5cc13ea83b9f9000917e.
> 
> 
> This patch was built on top of linux-5.14.0-rc1, and I successfully tested the patch
> in the following configurations:
> - disabled: no visible impact (as expected)
> - with ima_allowed_hashes=sha256,sha512:
> 	- alone: blocks both execution and xattr writes
> 	  (tested with `emvctl ima_hash -a md5 <my binary>`)
> 	- with ima_appraise=log: audit logs but still permits access
> 	- with ima_appraise=fix: audit logs but still permits access, however it
> 	  doesn't fix the hash (so maybe I should do something about it, because
> 	  right now it's basically the same as ima_appraise=log w.r.t. hash
> 	  algorithms ?)
> 	- with ima_accept_any_hash: permits access, no warnings whatsoever
> Do you want ideas of other configurations that I could test?
> 
> I would also like to point out that I'm more than open to suggestions for
> changing the names of the parameters (`ima_allowed_hashes` and
> `ima_accept_any_hash`) and the "cause" in the audit log (currently 
> forbidden-hash-algorithm"), because as you know, "naming things is hard".
> 
> 
> IMA: add an option to restrict the accepted hash algorithms
> 
> Adds two command-line parameters to limit the hash algorithms
> allowed for the security.ima xattr. 

Having two boot command line paramaters is a good indication that this
patch needs to be broken up.  Please refer to the section "Separate
your changes" in Documentation/process/submitting-patches.rst.  The
cover letter provides the motivation for the patch set.

> This gives users the
> ability to ensure their systems will not accept weak hashes,
> and potentially increase users trust in their IMA configuration,
> because they can ensure only strong collision-resistant hashes
> are employed and files generated otherwise will not be accepted.

Boot command line options should be minimized as much as possible. 
Perhaps without defining new kernel boot paramaters there are some
additional checks that could be added.  For example, on a FIPS enabled
system, prevent writing non FIPS allowed file signatures, limit file
signature algorithms to those enabled on the system, define new policy
rules to limit the permitted hash algorithms.

thanks,

Mimi

> 
> The main point is to safeguard users from mislabelling their files
> when using userland utilities to update their files, e.g. evmctl
> (`evmctl ima_hash` defaults to sha1). Another possible target
> would be people that have deployed IMA years ago, possibly using
> algorithms that was then deemed sufficiently collision-resistant,
> but that proved to be weak with the passage of time (note that this
> could also happen in the future with algorithms considered safe today).
> This patch also provides a migration path for users.

> 
> The first parameter supplies a list of allowed algorithms to
> the kernel, restricting appraisal to files hashed with "strong"
> hash algorithms (by default IMA will keep accepting any hash
> algorithm, as enabling such a feature by default would both be
> backward-incompatible and very probably break real systems).
> The second parameter is an escape hatch that adds a weaker form
> of backward-compatibility: when activated, IMA apparaisal will
> keep working on files hashed with an algorithm not present in
> the list, but updates to these files or new writes to the security.ima
> xattr will enforce the selected hash algorithms. This may be useful
> to perform an online migration from one algorithm to another.
> 
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>

Re: [PATCH] IMA: add an option to restrict the accepted hash algorithms

[THOBY Simon]

Hi Mimi,

On 7/15/21 2:26 PM, Mimi Zohar wrote:
> Hi Simon,
> 
> On Tue, 2021-07-13 at 14:48 +0000, THOBY Simon wrote:
>> Sorry it took me some time before I could write a patch (in addition it's my
[snip]
>> 	- with ima_accept_any_hash: permits access, no warnings whatsoever
>> Do you want ideas of other configurations that I could test?
>>
>> I would also like to point out that I'm more than open to suggestions for
>> changing the names of the parameters (`ima_allowed_hashes` and
>> `ima_accept_any_hash`) and the "cause" in the audit log (currently 
>> forbidden-hash-algorithm"), because as you know, "naming things is hard".
>>
>>
>> IMA: add an option to restrict the accepted hash algorithms
>>
>> Adds two command-line parameters to limit the hash algorithms
>> allowed for the security.ima xattr. 
> 
> Having two boot command line paramaters is a good indication that this
> patch needs to be broken up.  Please refer to the section "Separate
> your changes" in Documentation/process/submitting-patches.rst.  The
> cover letter provides the motivation for the patch set.
> 

Thanks for the clarification, I will split up the patch if a similar form
with two different options is kept.

>> This gives users the
>> ability to ensure their systems will not accept weak hashes,
>> and potentially increase users trust in their IMA configuration,
>> because they can ensure only strong collision-resistant hashes
>> are employed and files generated otherwise will not be accepted.
> 
> Boot command line options should be minimized as much as possible. 
> Perhaps without defining new kernel boot paramaters there are some
> additional checks that could be added.  For example, on a FIPS enabled
> system, prevent writing non FIPS allowed file signatures, limit file
> signature algorithms to those enabled on the system, define new policy
> rules to limit the permitted hash algorithms.

I understand the desire to keep the kernel parameter list as minimal as
possible, yet I'm not sure FIPS compliance targets the same use case.
I see this patch as potentially of interest for embedded products with
custom kernels (where "custom" here means a mainline kernel with more
security options enabled than in the kernels of most distros), and these
systems do not necessarily target FIPS compliance. In addition, FIPS 140-2
is the latest released version of FIPS 140 from what I could gather - I
didn't quite understand how far in the standardization process FIPS 140-3
was, but it doesn't seem totally public and/or deployed in the wild yet.
And FIPS 140-2 still permits use of SHA1 (not just HMAC-SHA, but the digest
algorithm too, see https://docs.oracle.com/cd/E53394_01/html/E54966/fips-refs.html)
but one of the points of this patchset was to allow people to enforce
stronger algorithms than SHA1 for IMA. This is why I am unsure that restricting
this mechanism to FIPS-enabled systems is the best approach. However, it
could still be interesting to enforce FIPS 140-2 authorized algorithms in
IMA, so maybe that could be a followup patch ? BTW, that may require
adding a list of the FIPS allowed algorithms to the kernel, as I haven't
found such a list when grepping "fips" through the kernel codebase.

Off the top of my head, I see three main alternatives to expose this
functionality:
1) Instead of supplying them at runtime in the kernel cmdline, add compile-time
toggles that hardcode in the kernel the list of authorized algorithms and the
expected behavior (verifying on write and/or verifying on appraisal). It would
basically be the same thing as the proposed patch, but without involving new
command line parameters. 
2) Use the ima_hash paramter as the sole authorized algorithm. I suppose this
is what you meant by "limit file signature algorithms to those enabled on the
system, unless you meant the algorithms built in the kernel with the
CRYPTO_* compile options ?
3) As you suggested, extend the policy DSL to limit the permitted hash
algorithms. This yields the added advantage of flexibility: you can decide
to restrict hashes for only some category of files/file systems, even though
I'm don't see many use cases for such flexibility (but I hold no doubt
someone somewhere would find some use to it).

I would love to know if you think one of those alternatives would be better
suited that the others for IMA, or if you thought of another option.

As a side note, I would also like to thank profusely Lakshmi for his remarks
on the patch  description and the process itself, as they will prove quite
valuable when submitting new revisions of this patch!


Thanks,
Simon

Re: [PATCH] IMA: add an option to restrict the accepted hash algorithms

[Mimi Zohar]

Hi Simon,

On Fri, 2021-07-16 at 09:18 +0000, THOBY Simon wrote:
> Off the top of my head, I see three main alternatives to expose this
> functionality:
> 1) Instead of supplying them at runtime in the kernel cmdline, add compile-time
> toggles that hardcode in the kernel the list of authorized algorithms and the
> expected behavior (verifying on write and/or verifying on appraisal). It would
> basically be the same thing as the proposed patch, but without involving new
> command line parameters. 

We also want to avoid defining new kernel Kconfig options, as much as
possible.

> 2) Use the ima_hash paramter as the sole authorized algorithm. I suppose this
> is what you meant by "limit file signature algorithms to those enabled on the
> system, unless you meant the algorithms built in the kernel with the
> CRYPTO_* compile options ?

By "limit file signature algorithms to those enabled on the system", I
was referring to the latter, not the"ima_hash" parameter.   That would
at least prevent writing file signatures based on unsupported hash
algorithms.   Thinking about it some more, the "ima_hash" might work
for some cases.  Continued below.

> 3) As you suggested, extend the policy DSL to limit the permitted hash
> algorithms. This yields the added advantage of flexibility: you can decide
> to restrict hashes for only some category of files/file systems, even though
> I'm don't see many use cases for such flexibility (but I hold no doubt
> someone somewhere would find some use to it).

In the embedded case, it's unlikely that different files would use
different hash algorithms for files signatures, but in the general case
different software packages could contain file signatures based on
different hash algorithms.

Defining an appraise rule "hash" option for enforcing sounds good.  The
"hash" option would be an allow list, which could be used for
transitioning from one hash algorithm to another.

In terms of writing the file signatures, the simpliest solution would
be to limit it to the "ima_hash" algorithm.   This might work in the
embedded case, but for the more general case it wouldn't.

> I would love to know if you think one of those alternatives would be better
> suited that the others for IMA, or if you thought of another option.

> As a side note, I would also like to thank profusely Lakshmi for his remarks
> on the patch  description and the process itself, as they will prove quite
> valuable when submitting new revisions of this patch!

Indeed, thanks Lakshmi!

thanks,

Mimi

Re: [PATCH] IMA: add an option to restrict the accepted hash algorithms

[Lakshmi Ramasubramanian]

Hi Simon,

On 7/13/2021 7:48 AM, THOBY Simon wrote:

Since you had stated that this is your first kernel contribution, I 
wanted to give you some ideas on how to organize the code changes, how 
to write patch description, etc. I hope that helps.

> This patch was built on top of linux-5.14.0-rc1, and I successfully tested the patch
> in the following configurations:
> - disabled: no visible impact (as expected)
> - with ima_allowed_hashes=sha256,sha512:
> 	- alone: blocks both execution and xattr writes
> 	  (tested with `emvctl ima_hash -a md5 <my binary>`)
> 	- with ima_appraise=log: audit logs but still permits access
> 	- with ima_appraise=fix: audit logs but still permits access, however it
> 	  doesn't fix the hash (so maybe I should do something about it, because
> 	  right now it's basically the same as ima_appraise=log w.r.t. hash
> 	  algorithms ?)
> 	- with ima_accept_any_hash: permits access, no warnings whatsoever
> Do you want ideas of other configurations that I could test?

As Mimi had stated, you should split the code changes into smaller 
patches and include a cover letter for the patch set.

In the cover letter you can describe the problem you are addressing, 
provide the motivation for the change and a more detailed description of 
the feature.

And, in the individual patches describe the code changes specific to 
that patch.

Please take a look at the cover letter and patches in the patch set 
Tushar Sugandhi had authored for "measuring kernel integrity critical 
data using IMA" (Links given below):

https://lore.kernel.org/linux-integrity/20210108040708.8389-1-tusharsu@linux.microsoft.com/

https://patchwork.kernel.org/project/linux-integrity/patch/20210108040708.8389-2-tusharsu@linux.microsoft.com/

Split the code changes into smaller patches with each patch building on 
top of the previous patch(es). You can look at Tushar's patch set to get 
an idea on how to organize the patches in a logical way.

> 
> I would also like to point out that I'm more than open to suggestions for
> changing the names of the parameters (`ima_allowed_hashes` and
> `ima_accept_any_hash`) and the "cause" in the audit log (currently
> forbidden-hash-algorithm"), because as you know, "naming things is hard".
> 
> 
> IMA: add an option to restrict the accepted hash algorithms
> 
> Adds two command-line parameters to limit the hash algorithms
> allowed for the security.ima xattr. This gives users the
> ability to ensure their systems will not accept weak hashes,

The patch description should first state what the problem is that you 
are fixing. For example, the patch to "generalize keyring specific 
measurement constructs" states:

IMA functions such as ima_match_keyring(), process_buffer_measurement(),
ima_match_policy() etc.  handle data specific to keyrings.  Currently,
these constructs are not generic to handle any func specific data.
This makes it harder to extend them without code duplication.

Then, describe the change to address the issue. The patch description 
should be written in imperative style (as if you are issuing commands to 
do certain actions). For example, to address the above issue in the 
current code that makes it hard to extend without code duplication, what 
action is to be performed:

Refactor the keyring specific measurement constructs to be generic and
reusable in other measurement scenarios.

With respect to the actual code change, please look into the comment 
Mimi had given earlier (copied below from her email) on minimizing boot 
command line options:

Boot command line options should be minimized as much as possible.
Perhaps without defining new kernel boot paramaters there are some
additional checks that could be added.  For example, on a FIPS enabled
system, prevent writing non FIPS allowed file signatures, limit file
signature algorithms to those enabled on the system, define new policy
rules to limit the permitted hash algorithms.

thanks,
  -lakshmi

> and potentially increase users trust in their IMA configuration,
> because they can ensure only strong collision-resistant hashes
> are employed and files generated otherwise will not be accepted.
> 
> The main point is to safeguard users from mislabelling their files
> when using userland utilities to update their files, e.g. evmctl
> (`evmctl ima_hash` defaults to sha1). Another possible target
> would be people that have deployed IMA years ago, possibly using
> algorithms that was then deemed sufficiently collision-resistant,
> but that proved to be weak with the passage of time (note that this
> could also happen in the future with algorithms considered safe today).
> This patch also provides a migration path for users.
> 
> The first parameter supplies a list of allowed algorithms to
> the kernel, restricting appraisal to files hashed with "strong"
> hash algorithms (by default IMA will keep accepting any hash
> algorithm, as enabling such a feature by default would both be
> backward-incompatible and very probably break real systems).
> The second parameter is an escape hatch that adds a weaker form
> of backward-compatibility: when activated, IMA apparaisal will
> keep working on files hashed with an algorithm not present in
> the list, but updates to these files or new writes to the security.ima
> xattr will enforce the selected hash algorithms. This may be useful
> to perform an online migration from one algorithm to another.
> 
> 
> Signed-off-by: Simon Thoby <simon.thoby@viveris.fr>
> ---
>   .../admin-guide/kernel-parameters.txt         | 42 +++++++++
>   security/integrity/ima/ima.h                  |  6 +-
>   security/integrity/ima/ima_appraise.c         | 22 +++++
>   security/integrity/ima/ima_main.c             | 88 ++++++++++++++++++-
>   4 files changed, 156 insertions(+), 2 deletions(-)