[RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

The first write on the ima policy file permits to override the default
policy defined with the ima_policy= boot parameter. This can be done
by adding the /etc/ima/ima-policy which allows loading the custom policy
during boot. It is also possible to load custom policy at runtime through
file operations:

cp custom_ima_policy /sys/kernel/security/ima/policy
cat custom_ima_policy > /sys/kernel/security/ima/policy

or by writing the absolute path of the file containing the custom policy:

echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy

In these cases, file signature can be necessary to load the policy
(func=POLICY_CHECK). Custom policy can also be set at runtime by directly
writing the policy stream on the ima policy file:

echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
 	"audit func=BPRM_CHECK mask=MAY_EXEC\n" \
     > /sys/kernel/security/ima/policy

In this case, there is no mechanism to verify the integrity of the new
policy.

Add a new entry in the ima measurements list containing the ascii custom
ima policy buffer when not verified at load time.

Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
---
 security/integrity/ima/ima.h        |  3 ++
 security/integrity/ima/ima_fs.c     | 11 ++++
 security/integrity/ima/ima_policy.c | 81 ++++++++++++++++++++++++++++-
 3 files changed, 93 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index c0d3b716d11f..27cba2e612a5 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -46,6 +46,8 @@ enum tpm_pcrs { TPM_PCR0 = 0, TPM_PCR8 = 8, TPM_PCR10 = 10 };
 /* current content of the policy */
 extern int ima_policy_flag;
 
+extern bool override_ima_policy;
+
 /* bitset of digests algorithms allowed in the setxattr hook */
 extern atomic_t ima_setxattr_allowed_hash_algorithms;
 
@@ -414,6 +416,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
 void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
 void ima_policy_stop(struct seq_file *m, void *v);
 int ima_policy_show(struct seq_file *m, void *v);
+void ima_measure_override_policy(size_t file_len);
 
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE	0x01
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index e4a79a9b2d58..8c516de4aebe 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -309,6 +309,9 @@ static const struct file_operations ima_ascii_measurements_ops = {
 	.release = seq_release,
 };
 
+static size_t text_policy_len;
+bool override_ima_policy;
+
 static ssize_t ima_read_policy(char *path)
 {
 	void *data = NULL;
@@ -383,6 +386,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 		result = -EACCES;
 	} else {
 		result = ima_parse_add_rule(data);
+		text_policy_len += (result + 1);
 	}
 	mutex_unlock(&ima_write_mutex);
 out_free:
@@ -532,6 +536,10 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 	}
 
 	ima_update_policy();
+	if (unlikely(override_ima_policy && text_policy_len)) {
+		ima_measure_override_policy(text_policy_len);
+		override_ima_policy = false;
+	}
 #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
 	securityfs_remove(ima_policy);
 	ima_policy = NULL;
@@ -558,6 +566,9 @@ int __init ima_fs_init(void)
 	ascii_securityfs_measurement_lists = NULL;
 	binary_securityfs_measurement_lists = NULL;
 
+	text_policy_len = 0;
+	override_ima_policy = false;
+
 	ima_dir = securityfs_create_dir("ima", integrity_dir);
 	if (IS_ERR(ima_dir))
 		return PTR_ERR(ima_dir);
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 21a8e54c383f..34753bce4668 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/rculist.h>
 #include <linux/seq_file.h>
+#include <linux/vmalloc.h>
 #include <linux/ima.h>
 
 #include "ima.h"
@@ -1044,6 +1045,8 @@ void ima_update_policy(void)
 	if (ima_rules != (struct list_head __rcu *)policy) {
 		ima_policy_flag = 0;
 
+		override_ima_policy = true;
+
 		rcu_assign_pointer(ima_rules, policy);
 		/*
 		 * IMA architecture specific policy rules are specified
@@ -1982,7 +1985,6 @@ const char *const func_tokens[] = {
 	__ima_hooks(__ima_hook_stringify)
 };
 
-#ifdef	CONFIG_IMA_READ_POLICY
 enum {
 	mask_exec = 0, mask_write, mask_read, mask_append
 };
@@ -1994,6 +1996,7 @@ static const char *const mask_tokens[] = {
 	"^MAY_APPEND"
 };
 
+#ifdef	CONFIG_IMA_READ_POLICY
 void *ima_policy_start(struct seq_file *m, loff_t *pos)
 {
 	loff_t l = *pos;
@@ -2028,6 +2031,7 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos)
 void ima_policy_stop(struct seq_file *m, void *v)
 {
 }
+#endif	/* CONFIG_IMA_READ_POLICY */
 
 #define pt(token)	policy_tokens[token].pattern
 #define mt(token)	mask_tokens[token]
@@ -2276,7 +2280,6 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
-#endif	/* CONFIG_IMA_READ_POLICY */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
 /*
@@ -2333,3 +2336,77 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
 	return found;
 }
 #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
+
+void ima_measure_override_policy(size_t file_len)
+{
+	struct ima_iint_cache iint = {};
+	const char event_name[] = "ima-policy-override";
+	struct ima_event_data event_data = {.iint = &iint,
+					    .filename = event_name};
+	struct ima_max_digest_data hash;
+	struct ima_digest_data *hash_hdr = container_of(&hash.hdr,
+						struct ima_digest_data, hdr);
+	static const char op[] = "measure_ima_policy_override";
+	struct ima_template_entry *entry = NULL;
+	static char *audit_cause = "ENOMEM";
+	struct ima_template_desc *template;
+	struct ima_rule_entry *rule_entry;
+	struct list_head *ima_rules_tmp;
+	struct seq_file file;
+	int result = -ENOMEM;
+	int violation = 0;
+
+	file.buf = vmalloc(file_len);
+	if (!file.buf)
+		goto out;
+
+	file.read_pos = 0;
+	file.size = file_len;
+	file.count = 0;
+
+	rcu_read_lock();
+	ima_rules_tmp = rcu_dereference(ima_rules);
+	list_for_each_entry_rcu(rule_entry, ima_rules_tmp, list) {
+		ima_policy_show(&file, rule_entry);
+	}
+	rcu_read_unlock();
+
+	event_data.buf = file.buf;
+	event_data.buf_len = file.count;
+
+	template = ima_template_desc_buf();
+	if (!template) {
+		audit_cause = "ima_template_desc_buf";
+		goto out_free;
+	}
+
+	iint.ima_hash = hash_hdr;
+	iint.ima_hash->algo = ima_hash_algo;
+	iint.ima_hash->length = hash_digest_size[ima_hash_algo];
+
+	result = ima_calc_buffer_hash(file.buf, file.count, iint.ima_hash);
+	if (result < 0) {
+		audit_cause = "hashing_error";
+		goto out_free;
+	}
+
+	result = ima_alloc_init_template(&event_data, &entry, template);
+	if (result < 0) {
+		audit_cause = "alloc_entry";
+		goto out_free;
+	}
+
+	result = ima_store_template(entry, violation, NULL, event_data.buf,
+				    CONFIG_IMA_MEASURE_PCR_IDX);
+	if (result < 0) {
+		audit_cause = "store_entry";
+		ima_free_template_entry(entry);
+	}
+
+out_free:
+	kvfree(file.buf);
+out:
+	if (result < 0)
+		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name,
+				    op, audit_cause, result, 1);
+}

base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
-- 
2.47.1

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Mimi Zohar]

On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> The first write on the ima policy file permits to override the default
> policy defined with the ima_policy= boot parameter. This can be done
> by adding the /etc/ima/ima-policy which allows loading the custom policy
> during boot. It is also possible to load custom policy at runtime through
> file operations:
> 
> cp custom_ima_policy /sys/kernel/security/ima/policy
> cat custom_ima_policy > /sys/kernel/security/ima/policy
> 
> or by writing the absolute path of the file containing the custom policy:
> 
> echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> 
> In these cases, file signature can be necessary to load the policy
> (func=POLICY_CHECK). Custom policy can also be set at runtime by directly
> writing the policy stream on the ima policy file:
> 
> echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
>  	"audit func=BPRM_CHECK mask=MAY_EXEC\n" \
>      > /sys/kernel/security/ima/policy
> 
> In this case, there is no mechanism to verify the integrity of the new
> policy.
> 
> Add a new entry in the ima measurements list containing the ascii custom
> ima policy buffer when not verified at load time.
> 
> Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>

Hi Enrico,

This patch set hard codes measuring the initial custom IMA policy rules that
replace the builtin policies specified on the boot command line.  IMA shouldn't
hard code policy.  I'm not quite sure why you're differentiating between
measuring the initial and subsequent custom IMA policy rules.  Consider defining
a new critical-data record to measure the current IMA policy rules.  Also
consider including the new critical-data rule in the arch specific policy rules.

thanks,

Mimi

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > The first write on the ima policy file permits to override the default
> > policy defined with the ima_policy= boot parameter. This can be done
> > by adding the /etc/ima/ima-policy which allows loading the custom policy
> > during boot. It is also possible to load custom policy at runtime through
> > file operations:
> > 
> > cp custom_ima_policy /sys/kernel/security/ima/policy
> > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > 
> > or by writing the absolute path of the file containing the custom policy:
> > 
> > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > 
> > In these cases, file signature can be necessary to load the policy
> > (func=POLICY_CHECK). Custom policy can also be set at runtime by directly
> > writing the policy stream on the ima policy file:
> > 
> > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> >      > /sys/kernel/security/ima/policy
> > 
> > In this case, there is no mechanism to verify the integrity of the new
> > policy.
> > 
> > Add a new entry in the ima measurements list containing the ascii custom
> > ima policy buffer when not verified at load time.
> > 
> > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> 
> Hi Enrico,

Hi Mimi,

thank you for the quick response.

> This patch set hard codes measuring the initial custom IMA policy rules that
> replace the builtin policies specified on the boot command line.  IMA
> shouldn't hard code policy.

My first approach was to define a new critical-data record, but performing the
measurement after the custom policy becomes effective, the measurement could be
bypassed omitting func=CRITICAL_DATA in the custom policy.

> I'm not quite sure why you're differentiating between
> measuring the initial and subsequent custom IMA policy rules.  

My intention is to measure the first direct write (line by line) on the policy
file, without loading the initial custom policy from a file. This case, if I'm
not wrong, is not covered by func=POLICY_CHECK.

> Consider defining a new critical-data record to measure the current IMA policy
> rules.  Also consider including the new critical-data rule in the arch
> specific policy rules.

I think that your suggestion, to add the critical-data rule in the arch policy
rules, solves the problems of bypassing the measurement and hard coding policy.

Thank you very much for your feedback.

Enrico

> thanks,
> 
> Mimi
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Mimi Zohar]

On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > The first write on the ima policy file permits to override the default
> > > policy defined with the ima_policy= boot parameter. This can be done
> > > by adding the /etc/ima/ima-policy which allows loading the custom policy
> > > during boot. It is also possible to load custom policy at runtime through
> > > file operations:
> > > 
> > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > 
> > > or by writing the absolute path of the file containing the custom policy:
> > > 
> > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > > 
> > > In these cases, file signature can be necessary to load the policy
> > > (func=POLICY_CHECK). Custom policy can also be set at runtime by directly
> > > writing the policy stream on the ima policy file:
> > > 
> > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > >      > /sys/kernel/security/ima/policy
> > > 
> > > In this case, there is no mechanism to verify the integrity of the new
> > > policy.
> > > 
> > > Add a new entry in the ima measurements list containing the ascii custom
> > > ima policy buffer when not verified at load time.
> > > 
> > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > 
> > Hi Enrico,
> 
> Hi Mimi,
> 
> thank you for the quick response.
> 
> > This patch set hard codes measuring the initial custom IMA policy rules that
> > replace the builtin policies specified on the boot command line.  IMA
> > shouldn't hard code policy.
> 
> My first approach was to define a new critical-data record, 

Hopefully the new critical-data will be of the entire IMA policy.

> but performing the
> measurement after the custom policy becomes effective, the measurement could be
> bypassed omitting func=CRITICAL_DATA in the custom policy.
> 
> > I'm not quite sure why you're differentiating between
> > measuring the initial and subsequent custom IMA policy rules.  
> 
> My intention is to measure the first direct write (line by line) on the policy
> file, without loading the initial custom policy from a file. This case, if I'm
> not wrong, is not covered by func=POLICY_CHECK.

When secure boot is enabled, the arch specific policy rules require the IMA
policy to be signed.  Without secure boot enabled, you're correct. The custom
policy rules may directly be loaded without being measured.

Why only measure "the first direct write"?  Additional custom policy rules may
be directly appended without being measured.

> 
> > Consider defining a new critical-data record to measure the current IMA policy
> > rules.  Also consider including the new critical-data rule in the arch
> > specific policy rules.
> 
> I think that your suggestion, to add the critical-data rule in the arch policy
> rules, solves the problems of bypassing the measurement and hard coding policy.
> 
> Thank you very much for your feedback.

You're welcome.

Mimi
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > The first write on the ima policy file permits to override the default
> > > > policy defined with the ima_policy= boot parameter. This can be done
> > > > by adding the /etc/ima/ima-policy which allows loading the custom policy
> > > > during boot. It is also possible to load custom policy at runtime
> > > > through
> > > > file operations:
> > > > 
> > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > 
> > > > or by writing the absolute path of the file containing the custom
> > > > policy:
> > > > 
> > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > > > 
> > > > In these cases, file signature can be necessary to load the policy
> > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > directly
> > > > writing the policy stream on the ima policy file:
> > > > 
> > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > >      > /sys/kernel/security/ima/policy
> > > > 
> > > > In this case, there is no mechanism to verify the integrity of the new
> > > > policy.
> > > > 
> > > > Add a new entry in the ima measurements list containing the ascii custom
> > > > ima policy buffer when not verified at load time.
> > > > 
> > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > 
> > > Hi Enrico,
> > 
> > Hi Mimi,
> > 
> > thank you for the quick response.
> > 
> > > This patch set hard codes measuring the initial custom IMA policy rules
> > > that
> > > replace the builtin policies specified on the boot command line.  IMA
> > > shouldn't hard code policy.
> > 
> > My first approach was to define a new critical-data record, 

Hi Mimi,

> Hopefully the new critical-data will be of the entire IMA policy.

yes, absolutely.

> > but performing the
> > measurement after the custom policy becomes effective, the measurement could
> > be
> > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > 
> > > I'm not quite sure why you're differentiating between
> > > measuring the initial and subsequent custom IMA policy rules.  
> > 
> > My intention is to measure the first direct write (line by line) on the
> > policy
> > file, without loading the initial custom policy from a file. This case, if
> > I'm
> > not wrong, is not covered by func=POLICY_CHECK.
> 
> When secure boot is enabled, the arch specific policy rules require the IMA
> policy to be signed.  Without secure boot enabled, you're correct. The custom
> policy rules may directly be loaded without being measured.
> 
> Why only measure "the first direct write"?  Additional custom policy rules may
> be directly appended without being measured.

Yes, you right. The aim was to measure (at least) the first one, because it
substitutes the boot policy, but if you are ok with adding a critical-data
record, it would be definitely better.

Thank you,

Enrico

> > 
> > > Consider defining a new critical-data record to measure the current IMA
> > > policy
> > > rules.  Also consider including the new critical-data rule in the arch
> > > specific policy rules.
> > 
> > I think that your suggestion, to add the critical-data rule in the arch
> > policy
> > rules, solves the problems of bypassing the measurement and hard coding
> > policy.
> > 
> > Thank you very much for your feedback.
> 
> You're welcome.
> 
> Mimi
> > 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Roberto Sassu]

On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > The first write on the ima policy file permits to override the default
> > > > > policy defined with the ima_policy= boot parameter. This can be done
> > > > > by adding the /etc/ima/ima-policy which allows loading the custom policy
> > > > > during boot. It is also possible to load custom policy at runtime
> > > > > through
> > > > > file operations:
> > > > > 
> > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > 
> > > > > or by writing the absolute path of the file containing the custom
> > > > > policy:
> > > > > 
> > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > 
> > > > > In these cases, file signature can be necessary to load the policy
> > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > > directly
> > > > > writing the policy stream on the ima policy file:
> > > > > 
> > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > >      > /sys/kernel/security/ima/policy
> > > > > 
> > > > > In this case, there is no mechanism to verify the integrity of the new
> > > > > policy.
> > > > > 
> > > > > Add a new entry in the ima measurements list containing the ascii custom
> > > > > ima policy buffer when not verified at load time.
> > > > > 
> > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > 
> > > > Hi Enrico,
> > > 
> > > Hi Mimi,
> > > 
> > > thank you for the quick response.
> > > 
> > > > This patch set hard codes measuring the initial custom IMA policy rules
> > > > that
> > > > replace the builtin policies specified on the boot command line.  IMA
> > > > shouldn't hard code policy.
> > > 
> > > My first approach was to define a new critical-data record, 
> 
> Hi Mimi,
> 
> > Hopefully the new critical-data will be of the entire IMA policy.
> 
> yes, absolutely.
> 
> > > but performing the
> > > measurement after the custom policy becomes effective, the measurement could
> > > be
> > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > 
> > > > I'm not quite sure why you're differentiating between
> > > > measuring the initial and subsequent custom IMA policy rules.  
> > > 
> > > My intention is to measure the first direct write (line by line) on the
> > > policy
> > > file, without loading the initial custom policy from a file. This case, if
> > > I'm
> > > not wrong, is not covered by func=POLICY_CHECK.
> > 
> > When secure boot is enabled, the arch specific policy rules require the IMA
> > policy to be signed.  Without secure boot enabled, you're correct. The custom
> > policy rules may directly be loaded without being measured.
> > 
> > Why only measure "the first direct write"?  Additional custom policy rules may
> > be directly appended without being measured.
> 
> Yes, you right. The aim was to measure (at least) the first one, because it
> substitutes the boot policy, but if you are ok with adding a critical-data
> record, it would be definitely better.

Hi Enrico

in addition to what Mimi suggested, I also like to idea that the
POLICY_CHECK hook catches the direct policy loading. That would mean
that those updates would be seen if the 'tcb' IMA policy is selected.

I would have recommended to try to add a process_measurement() call in
ima_write_policy(), where the buffer to be processed is.

However, I guess you need to have a valid file descriptor in order to
use that function (maybe an anonymous inode?).
ima_collect_measurement() should be already able to handle buffers,
passed by ima_post_read_file().

Thanks

Roberto

> Thank you,
> 
> Enrico
> 
> > > 
> > > > Consider defining a new critical-data record to measure the current IMA
> > > > policy
> > > > rules.  Also consider including the new critical-data rule in the arch
> > > > specific policy rules.
> > > 
> > > I think that your suggestion, to add the critical-data rule in the arch
> > > policy
> > > rules, solves the problems of bypassing the measurement and hard coding
> > > policy.
> > > 
> > > Thank you very much for your feedback.
> > 
> > You're welcome.
> > 
> > Mimi
> > > 
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

On Thu, 2025-02-27 at 15:49 +0100, Roberto Sassu wrote:
> On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > > The first write on the ima policy file permits to override the
> > > > > > default
> > > > > > policy defined with the ima_policy= boot parameter. This can be done
> > > > > > by adding the /etc/ima/ima-policy which allows loading the custom
> > > > > > policy
> > > > > > during boot. It is also possible to load custom policy at runtime
> > > > > > through
> > > > > > file operations:
> > > > > > 
> > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > 
> > > > > > or by writing the absolute path of the file containing the custom
> > > > > > policy:
> > > > > > 
> > > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > 
> > > > > > In these cases, file signature can be necessary to load the policy
> > > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > > > directly
> > > > > > writing the policy stream on the ima policy file:
> > > > > > 
> > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > >      > /sys/kernel/security/ima/policy
> > > > > > 
> > > > > > In this case, there is no mechanism to verify the integrity of the
> > > > > > new
> > > > > > policy.
> > > > > > 
> > > > > > Add a new entry in the ima measurements list containing the ascii
> > > > > > custom
> > > > > > ima policy buffer when not verified at load time.
> > > > > > 
> > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > > 
> > > > > Hi Enrico,
> > > > 
> > > > Hi Mimi,
> > > > 
> > > > thank you for the quick response.
> > > > 
> > > > > This patch set hard codes measuring the initial custom IMA policy
> > > > > rules
> > > > > that
> > > > > replace the builtin policies specified on the boot command line.  IMA
> > > > > shouldn't hard code policy.
> > > > 
> > > > My first approach was to define a new critical-data record, 
> > 
> > Hi Mimi,
> > 
> > > Hopefully the new critical-data will be of the entire IMA policy.
> > 
> > yes, absolutely.
> > 
> > > > but performing the
> > > > measurement after the custom policy becomes effective, the measurement
> > > > could
> > > > be
> > > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > > 
> > > > > I'm not quite sure why you're differentiating between
> > > > > measuring the initial and subsequent custom IMA policy rules.  
> > > > 
> > > > My intention is to measure the first direct write (line by line) on the
> > > > policy
> > > > file, without loading the initial custom policy from a file. This case,
> > > > if
> > > > I'm
> > > > not wrong, is not covered by func=POLICY_CHECK.
> > > 
> > > When secure boot is enabled, the arch specific policy rules require the
> > > IMA
> > > policy to be signed.  Without secure boot enabled, you're correct. The
> > > custom
> > > policy rules may directly be loaded without being measured.
> > > 
> > > Why only measure "the first direct write"?  Additional custom policy rules
> > > may
> > > be directly appended without being measured.
> > 
> > Yes, you right. The aim was to measure (at least) the first one, because it
> > substitutes the boot policy, but if you are ok with adding a critical-data
> > record, it would be definitely better.
> 
> Hi Enrico
> 
> in addition to what Mimi suggested, I also like to idea that the
> POLICY_CHECK hook catches the direct policy loading. That would mean
> that those updates would be seen if the 'tcb' IMA policy is selected.

Hi Roberto,

in this case, wouldn't be used the current template? Wouldn't be better to use
the ima-buf in order to include the textual policy representation?

In addition, there would be a new record for each line of the input buffer, and
measuring the input buffer would produce different measurements for the same
resulting policy entry, because different or multiple separators can be used.

I opted to perform the measurement in ima_release_policy() because is where the
new policy becomes effective after ima_update_policy() and can be done a single
measurement of the new running policy.

The measurement could be done a bit earlier, working on ima_policy_rules and
ima_temp_rules (which basically contains the input buffer) before the splicing,
so it would be considered the current policy and not the new one. In this case,
it would work also when ima_policy=tcb is set, and it could be called
process_buffer_measurement() with POLICY_CHECK, to get a record with the entire
IMA policy.
What do you think about it?

BR,

Enrico

> I would have recommended to try to add a process_measurement() call in
> ima_write_policy(), where the buffer to be processed is.
> 
> However, I guess you need to have a valid file descriptor in order to
> use that function (maybe an anonymous inode?).
> ima_collect_measurement() should be already able to handle buffers,
> passed by ima_post_read_file().
> 
> Thanks
> 
> Roberto
> 
> > Thank you,
> > 
> > Enrico
> > 
> > > > 
> > > > > Consider defining a new critical-data record to measure the current
> > > > > IMA
> > > > > policy
> > > > > rules.  Also consider including the new critical-data rule in the arch
> > > > > specific policy rules.
> > > > 
> > > > I think that your suggestion, to add the critical-data rule in the arch
> > > > policy
> > > > rules, solves the problems of bypassing the measurement and hard coding
> > > > policy.
> > > > 
> > > > Thank you very much for your feedback.
> > > 
> > > You're welcome.
> > > 
> > > Mimi
> > > > 
> > 
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Roberto Sassu]

On Mon, 2025-03-03 at 10:26 +0000, Enrico  Bravi wrote:
> On Thu, 2025-02-27 at 15:49 +0100, Roberto Sassu wrote:
> > On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> > > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > > > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > > > The first write on the ima policy file permits to override the
> > > > > > > default
> > > > > > > policy defined with the ima_policy= boot parameter. This can be done
> > > > > > > by adding the /etc/ima/ima-policy which allows loading the custom
> > > > > > > policy
> > > > > > > during boot. It is also possible to load custom policy at runtime
> > > > > > > through
> > > > > > > file operations:
> > > > > > > 
> > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > > 
> > > > > > > or by writing the absolute path of the file containing the custom
> > > > > > > policy:
> > > > > > > 
> > > > > > > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > > 
> > > > > > > In these cases, file signature can be necessary to load the policy
> > > > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > > > > directly
> > > > > > > writing the policy stream on the ima policy file:
> > > > > > > 
> > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > >      > /sys/kernel/security/ima/policy
> > > > > > > 
> > > > > > > In this case, there is no mechanism to verify the integrity of the
> > > > > > > new
> > > > > > > policy.
> > > > > > > 
> > > > > > > Add a new entry in the ima measurements list containing the ascii
> > > > > > > custom
> > > > > > > ima policy buffer when not verified at load time.
> > > > > > > 
> > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > > > 
> > > > > > Hi Enrico,
> > > > > 
> > > > > Hi Mimi,
> > > > > 
> > > > > thank you for the quick response.
> > > > > 
> > > > > > This patch set hard codes measuring the initial custom IMA policy
> > > > > > rules
> > > > > > that
> > > > > > replace the builtin policies specified on the boot command line.  IMA
> > > > > > shouldn't hard code policy.
> > > > > 
> > > > > My first approach was to define a new critical-data record, 
> > > 
> > > Hi Mimi,
> > > 
> > > > Hopefully the new critical-data will be of the entire IMA policy.
> > > 
> > > yes, absolutely.
> > > 
> > > > > but performing the
> > > > > measurement after the custom policy becomes effective, the measurement
> > > > > could
> > > > > be
> > > > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > > > 
> > > > > > I'm not quite sure why you're differentiating between
> > > > > > measuring the initial and subsequent custom IMA policy rules.  
> > > > > 
> > > > > My intention is to measure the first direct write (line by line) on the
> > > > > policy
> > > > > file, without loading the initial custom policy from a file. This case,
> > > > > if
> > > > > I'm
> > > > > not wrong, is not covered by func=POLICY_CHECK.
> > > > 
> > > > When secure boot is enabled, the arch specific policy rules require the
> > > > IMA
> > > > policy to be signed.  Without secure boot enabled, you're correct. The
> > > > custom
> > > > policy rules may directly be loaded without being measured.
> > > > 
> > > > Why only measure "the first direct write"?  Additional custom policy rules
> > > > may
> > > > be directly appended without being measured.
> > > 
> > > Yes, you right. The aim was to measure (at least) the first one, because it
> > > substitutes the boot policy, but if you are ok with adding a critical-data
> > > record, it would be definitely better.
> > 
> > Hi Enrico
> > 
> > in addition to what Mimi suggested, I also like to idea that the
> > POLICY_CHECK hook catches the direct policy loading. That would mean
> > that those updates would be seen if the 'tcb' IMA policy is selected.
> 
> Hi Roberto,
> 
> in this case, wouldn't be used the current template? Wouldn't be better to use
> the ima-buf in order to include the textual policy representation?

Hi Enrico

I would use the current template, I don't find any particular issues
for it. Sure, we don't have a file to measure but there are other cases
where in process_measurement() we measure a buffer instead of a file
(when it is called by ima_post_read_file()).

We can have both critical data and POLICY_CHECK measurement.

> In addition, there would be a new record for each line of the input buffer, and
> measuring the input buffer would produce different measurements for the same
> resulting policy entry, because different or multiple separators can be used.
> 
> I opted to perform the measurement in ima_release_policy() because is where the
> new policy becomes effective after ima_update_policy() and can be done a single
> measurement of the new running policy.

I would simply measure what is passed to ima_write_policy() regardless
of whether the policy will be accepted or not. This is more in line
with the trusted computing paradigm of measure & load. If potentially
there is a bug in the policy code, measuring the policy before with a
vulnerable kernel would allow you to see the measurement. After, it
depends on the seriousness of the vulnerability.

Roberto

> The measurement could be done a bit earlier, working on ima_policy_rules and
> ima_temp_rules (which basically contains the input buffer) before the splicing,
> so it would be considered the current policy and not the new one. In this case,
> it would work also when ima_policy=tcb is set, and it could be called
> process_buffer_measurement() with POLICY_CHECK, to get a record with the entire
> IMA policy.
> What do you think about it?
> 
> BR,
> 
> Enrico
> 
> > I would have recommended to try to add a process_measurement() call in
> > ima_write_policy(), where the buffer to be processed is.
> > 
> > However, I guess you need to have a valid file descriptor in order to
> > use that function (maybe an anonymous inode?).
> > ima_collect_measurement() should be already able to handle buffers,
> > passed by ima_post_read_file().
> > 
> > Thanks
> > 
> > Roberto
> > 
> > > Thank you,
> > > 
> > > Enrico
> > > 
> > > > > 
> > > > > > Consider defining a new critical-data record to measure the current
> > > > > > IMA
> > > > > > policy
> > > > > > rules.  Also consider including the new critical-data rule in the arch
> > > > > > specific policy rules.
> > > > > 
> > > > > I think that your suggestion, to add the critical-data rule in the arch
> > > > > policy
> > > > > rules, solves the problems of bypassing the measurement and hard coding
> > > > > policy.
> > > > > 
> > > > > Thank you very much for your feedback.
> > > > 
> > > > You're welcome.
> > > > 
> > > > Mimi
> > > > > 
> > > 
> > 
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

On Wed, 2025-03-05 at 09:59 +0100, Roberto Sassu wrote:
> On Mon, 2025-03-03 at 10:26 +0000, Enrico  Bravi wrote:
> > On Thu, 2025-02-27 at 15:49 +0100, Roberto Sassu wrote:
> > > On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> > > > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > > > > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > > > > The first write on the ima policy file permits to override the
> > > > > > > > default
> > > > > > > > policy defined with the ima_policy= boot parameter. This can be
> > > > > > > > done
> > > > > > > > by adding the /etc/ima/ima-policy which allows loading the
> > > > > > > > custom
> > > > > > > > policy
> > > > > > > > during boot. It is also possible to load custom policy at
> > > > > > > > runtime
> > > > > > > > through
> > > > > > > > file operations:
> > > > > > > > 
> > > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > > > 
> > > > > > > > or by writing the absolute path of the file containing the
> > > > > > > > custom
> > > > > > > > policy:
> > > > > > > > 
> > > > > > > > echo /path/of/custom_ima_policy >
> > > > > > > > /sys/kernel/security/ima/policy
> > > > > > > > 
> > > > > > > > In these cases, file signature can be necessary to load the
> > > > > > > > policy
> > > > > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > > > > > directly
> > > > > > > > writing the policy stream on the ima policy file:
> > > > > > > > 
> > > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > >      > /sys/kernel/security/ima/policy
> > > > > > > > 
> > > > > > > > In this case, there is no mechanism to verify the integrity of
> > > > > > > > the
> > > > > > > > new
> > > > > > > > policy.
> > > > > > > > 
> > > > > > > > Add a new entry in the ima measurements list containing the
> > > > > > > > ascii
> > > > > > > > custom
> > > > > > > > ima policy buffer when not verified at load time.
> > > > > > > > 
> > > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > > > > 
> > > > > > > Hi Enrico,
> > > > > > 
> > > > > > Hi Mimi,
> > > > > > 
> > > > > > thank you for the quick response.
> > > > > > 
> > > > > > > This patch set hard codes measuring the initial custom IMA policy
> > > > > > > rules
> > > > > > > that
> > > > > > > replace the builtin policies specified on the boot command line. 
> > > > > > > IMA
> > > > > > > shouldn't hard code policy.
> > > > > > 
> > > > > > My first approach was to define a new critical-data record, 
> > > > 
> > > > Hi Mimi,
> > > > 
> > > > > Hopefully the new critical-data will be of the entire IMA policy.
> > > > 
> > > > yes, absolutely.
> > > > 
> > > > > > but performing the
> > > > > > measurement after the custom policy becomes effective, the
> > > > > > measurement
> > > > > > could
> > > > > > be
> > > > > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > > > > 
> > > > > > > I'm not quite sure why you're differentiating between
> > > > > > > measuring the initial and subsequent custom IMA policy rules.  
> > > > > > 
> > > > > > My intention is to measure the first direct write (line by line) on
> > > > > > the
> > > > > > policy
> > > > > > file, without loading the initial custom policy from a file. This
> > > > > > case,
> > > > > > if
> > > > > > I'm
> > > > > > not wrong, is not covered by func=POLICY_CHECK.
> > > > > 
> > > > > When secure boot is enabled, the arch specific policy rules require
> > > > > the
> > > > > IMA
> > > > > policy to be signed.  Without secure boot enabled, you're correct. The
> > > > > custom
> > > > > policy rules may directly be loaded without being measured.
> > > > > 
> > > > > Why only measure "the first direct write"?  Additional custom policy
> > > > > rules
> > > > > may
> > > > > be directly appended without being measured.
> > > > 
> > > > Yes, you right. The aim was to measure (at least) the first one, because
> > > > it
> > > > substitutes the boot policy, but if you are ok with adding a critical-
> > > > data
> > > > record, it would be definitely better.
> > > 
> > > Hi Enrico
> > > 
> > > in addition to what Mimi suggested, I also like to idea that the
> > > POLICY_CHECK hook catches the direct policy loading. That would mean
> > > that those updates would be seen if the 'tcb' IMA policy is selected.
> > 
> > Hi Roberto,
> > 
> > in this case, wouldn't be used the current template? Wouldn't be better to
> > use
> > the ima-buf in order to include the textual policy representation?
> 
> Hi Enrico
> 
> I would use the current template, I don't find any particular issues
> for it. Sure, we don't have a file to measure but there are other cases
> where in process_measurement() we measure a buffer instead of a file
> (when it is called by ima_post_read_file()).
> 
> We can have both critical data and POLICY_CHECK measurement.

Hi Roberto,

sorry, I didn't get this point. What do you mean?

> > In addition, there would be a new record for each line of the input buffer,
> > and
> > measuring the input buffer would produce different measurements for the same
> > resulting policy entry, because different or multiple separators can be
> > used.
> > 
> > I opted to perform the measurement in ima_release_policy() because is where
> > the
> > new policy becomes effective after ima_update_policy() and can be done a
> > single
> > measurement of the new running policy.
> 
> I would simply measure what is passed to ima_write_policy() regardless
> of whether the policy will be accepted or not. This is more in line
> with the trusted computing paradigm of measure & load. If potentially
> there is a bug in the policy code, measuring the policy before with a
> vulnerable kernel would allow you to see the measurement. After, it
> depends on the seriousness of the vulnerability.

Ok perfect, I get your point. Thank you for the explanation.

Enrico

> Roberto
> 
> > The measurement could be done a bit earlier, working on ima_policy_rules and
> > ima_temp_rules (which basically contains the input buffer) before the
> > splicing,
> > so it would be considered the current policy and not the new one. In this
> > case,
> > it would work also when ima_policy=tcb is set, and it could be called
> > process_buffer_measurement() with POLICY_CHECK, to get a record with the
> > entire
> > IMA policy.
> > What do you think about it?
> > 
> > BR,
> > 
> > Enrico
> > 
> > > I would have recommended to try to add a process_measurement() call in
> > > ima_write_policy(), where the buffer to be processed is.
> > > 
> > > However, I guess you need to have a valid file descriptor in order to
> > > use that function (maybe an anonymous inode?).
> > > ima_collect_measurement() should be already able to handle buffers,
> > > passed by ima_post_read_file().
> > > 
> > > Thanks
> > > 
> > > Roberto
> > > 
> > > > Thank you,
> > > > 
> > > > Enrico
> > > > 
> > > > > > 
> > > > > > > Consider defining a new critical-data record to measure the
> > > > > > > current
> > > > > > > IMA
> > > > > > > policy
> > > > > > > rules.  Also consider including the new critical-data rule in the
> > > > > > > arch
> > > > > > > specific policy rules.
> > > > > > 
> > > > > > I think that your suggestion, to add the critical-data rule in the
> > > > > > arch
> > > > > > policy
> > > > > > rules, solves the problems of bypassing the measurement and hard
> > > > > > coding
> > > > > > policy.
> > > > > > 
> > > > > > Thank you very much for your feedback.
> > > > > 
> > > > > You're welcome.
> > > > > 
> > > > > Mimi
> > > > > > 
> > > > 
> > > 
> > 
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Roberto Sassu]

On Thu, 2025-03-06 at 08:20 +0000, Enrico  Bravi wrote:
> On Wed, 2025-03-05 at 09:59 +0100, Roberto Sassu wrote:
> > On Mon, 2025-03-03 at 10:26 +0000, Enrico  Bravi wrote:
> > > On Thu, 2025-02-27 at 15:49 +0100, Roberto Sassu wrote:
> > > > On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> > > > > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > > > > > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > > > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > > > > > The first write on the ima policy file permits to override the
> > > > > > > > > default
> > > > > > > > > policy defined with the ima_policy= boot parameter. This can be
> > > > > > > > > done
> > > > > > > > > by adding the /etc/ima/ima-policy which allows loading the
> > > > > > > > > custom
> > > > > > > > > policy
> > > > > > > > > during boot. It is also possible to load custom policy at
> > > > > > > > > runtime
> > > > > > > > > through
> > > > > > > > > file operations:
> > > > > > > > > 
> > > > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > > > > 
> > > > > > > > > or by writing the absolute path of the file containing the
> > > > > > > > > custom
> > > > > > > > > policy:
> > > > > > > > > 
> > > > > > > > > echo /path/of/custom_ima_policy >
> > > > > > > > > /sys/kernel/security/ima/policy
> > > > > > > > > 
> > > > > > > > > In these cases, file signature can be necessary to load the
> > > > > > > > > policy
> > > > > > > > > (func=POLICY_CHECK). Custom policy can also be set at runtime by
> > > > > > > > > directly
> > > > > > > > > writing the policy stream on the ima policy file:
> > > > > > > > > 
> > > > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > > >      > /sys/kernel/security/ima/policy
> > > > > > > > > 
> > > > > > > > > In this case, there is no mechanism to verify the integrity of
> > > > > > > > > the
> > > > > > > > > new
> > > > > > > > > policy.
> > > > > > > > > 
> > > > > > > > > Add a new entry in the ima measurements list containing the
> > > > > > > > > ascii
> > > > > > > > > custom
> > > > > > > > > ima policy buffer when not verified at load time.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > > > > > 
> > > > > > > > Hi Enrico,
> > > > > > > 
> > > > > > > Hi Mimi,
> > > > > > > 
> > > > > > > thank you for the quick response.
> > > > > > > 
> > > > > > > > This patch set hard codes measuring the initial custom IMA policy
> > > > > > > > rules
> > > > > > > > that
> > > > > > > > replace the builtin policies specified on the boot command line. 
> > > > > > > > IMA
> > > > > > > > shouldn't hard code policy.
> > > > > > > 
> > > > > > > My first approach was to define a new critical-data record, 
> > > > > 
> > > > > Hi Mimi,
> > > > > 
> > > > > > Hopefully the new critical-data will be of the entire IMA policy.
> > > > > 
> > > > > yes, absolutely.
> > > > > 
> > > > > > > but performing the
> > > > > > > measurement after the custom policy becomes effective, the
> > > > > > > measurement
> > > > > > > could
> > > > > > > be
> > > > > > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > > > > > 
> > > > > > > > I'm not quite sure why you're differentiating between
> > > > > > > > measuring the initial and subsequent custom IMA policy rules.  
> > > > > > > 
> > > > > > > My intention is to measure the first direct write (line by line) on
> > > > > > > the
> > > > > > > policy
> > > > > > > file, without loading the initial custom policy from a file. This
> > > > > > > case,
> > > > > > > if
> > > > > > > I'm
> > > > > > > not wrong, is not covered by func=POLICY_CHECK.
> > > > > > 
> > > > > > When secure boot is enabled, the arch specific policy rules require
> > > > > > the
> > > > > > IMA
> > > > > > policy to be signed.  Without secure boot enabled, you're correct. The
> > > > > > custom
> > > > > > policy rules may directly be loaded without being measured.
> > > > > > 
> > > > > > Why only measure "the first direct write"?  Additional custom policy
> > > > > > rules
> > > > > > may
> > > > > > be directly appended without being measured.
> > > > > 
> > > > > Yes, you right. The aim was to measure (at least) the first one, because
> > > > > it
> > > > > substitutes the boot policy, but if you are ok with adding a critical-
> > > > > data
> > > > > record, it would be definitely better.
> > > > 
> > > > Hi Enrico
> > > > 
> > > > in addition to what Mimi suggested, I also like to idea that the
> > > > POLICY_CHECK hook catches the direct policy loading. That would mean
> > > > that those updates would be seen if the 'tcb' IMA policy is selected.
> > > 
> > > Hi Roberto,
> > > 
> > > in this case, wouldn't be used the current template? Wouldn't be better to
> > > use
> > > the ima-buf in order to include the textual policy representation?
> > 
> > Hi Enrico
> > 
> > I would use the current template, I don't find any particular issues
> > for it. Sure, we don't have a file to measure but there are other cases
> > where in process_measurement() we measure a buffer instead of a file
> > (when it is called by ima_post_read_file()).
> > 
> > We can have both critical data and POLICY_CHECK measurement.

CRITICAL_DATA and POLICY_CHECK are separate hooks.

They can do measurement differently, CRITICAL_DATA can be for measuring
the current state of the full policy, while POLICY_CHECK could be for
policies sent to the kernel.


> Hi Roberto,
> 
> sorry, I didn't get this point. What do you mean?
> 
> > > In addition, there would be a new record for each line of the input buffer,
> > > and
> > > measuring the input buffer would produce different measurements for the same
> > > resulting policy entry, because different or multiple separators can be
> > > used.
> > > 
> > > I opted to perform the measurement in ima_release_policy() because is where
> > > the
> > > new policy becomes effective after ima_update_policy() and can be done a
> > > single
> > > measurement of the new running policy.
> > 
> > I would simply measure what is passed to ima_write_policy() regardless
> > of whether the policy will be accepted or not. This is more in line
> > with the trusted computing paradigm of measure & load. If potentially
> > there is a bug in the policy code, measuring the policy before with a
> > vulnerable kernel would allow you to see the measurement. After, it
> > depends on the seriousness of the vulnerability.
> 
> Ok perfect, I get your point. Thank you for the explanation.

Welcome.

Roberto

> Enrico
> 
> > Roberto
> > 
> > > The measurement could be done a bit earlier, working on ima_policy_rules and
> > > ima_temp_rules (which basically contains the input buffer) before the
> > > splicing,
> > > so it would be considered the current policy and not the new one. In this
> > > case,
> > > it would work also when ima_policy=tcb is set, and it could be called
> > > process_buffer_measurement() with POLICY_CHECK, to get a record with the
> > > entire
> > > IMA policy.
> > > What do you think about it?
> > > 
> > > BR,
> > > 
> > > Enrico
> > > 
> > > > I would have recommended to try to add a process_measurement() call in
> > > > ima_write_policy(), where the buffer to be processed is.
> > > > 
> > > > However, I guess you need to have a valid file descriptor in order to
> > > > use that function (maybe an anonymous inode?).
> > > > ima_collect_measurement() should be already able to handle buffers,
> > > > passed by ima_post_read_file().
> > > > 
> > > > Thanks
> > > > 
> > > > Roberto
> > > > 
> > > > > Thank you,
> > > > > 
> > > > > Enrico
> > > > > 
> > > > > > > 
> > > > > > > > Consider defining a new critical-data record to measure the
> > > > > > > > current
> > > > > > > > IMA
> > > > > > > > policy
> > > > > > > > rules.  Also consider including the new critical-data rule in the
> > > > > > > > arch
> > > > > > > > specific policy rules.
> > > > > > > 
> > > > > > > I think that your suggestion, to add the critical-data rule in the
> > > > > > > arch
> > > > > > > policy
> > > > > > > rules, solves the problems of bypassing the measurement and hard
> > > > > > > coding
> > > > > > > policy.
> > > > > > > 
> > > > > > > Thank you very much for your feedback.
> > > > > > 
> > > > > > You're welcome.
> > > > > > 
> > > > > > Mimi
> > > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

Re: [RFC][PATCH] ima: add measurement for first unverified write on ima policy file

[Enrico Bravi]

On Thu, 2025-03-06 at 09:47 +0100, Roberto Sassu wrote:
> On Thu, 2025-03-06 at 08:20 +0000, Enrico  Bravi wrote:
> > On Wed, 2025-03-05 at 09:59 +0100, Roberto Sassu wrote:
> > > On Mon, 2025-03-03 at 10:26 +0000, Enrico  Bravi wrote:
> > > > On Thu, 2025-02-27 at 15:49 +0100, Roberto Sassu wrote:
> > > > > On Thu, 2025-02-27 at 11:36 +0000, Enrico  Bravi wrote:
> > > > > > On Wed, 2025-02-26 at 22:05 -0500, Mimi Zohar wrote:
> > > > > > > On Wed, 2025-02-26 at 22:53 +0000, Enrico  Bravi wrote:
> > > > > > > > On Tue, 2025-02-25 at 20:53 -0500, Mimi Zohar wrote:
> > > > > > > > > On Tue, 2025-02-25 at 14:12 +0100, Enrico Bravi wrote:
> > > > > > > > > > The first write on the ima policy file permits to override
> > > > > > > > > > the
> > > > > > > > > > default
> > > > > > > > > > policy defined with the ima_policy= boot parameter. This can
> > > > > > > > > > be
> > > > > > > > > > done
> > > > > > > > > > by adding the /etc/ima/ima-policy which allows loading the
> > > > > > > > > > custom
> > > > > > > > > > policy
> > > > > > > > > > during boot. It is also possible to load custom policy at
> > > > > > > > > > runtime
> > > > > > > > > > through
> > > > > > > > > > file operations:
> > > > > > > > > > 
> > > > > > > > > > cp custom_ima_policy /sys/kernel/security/ima/policy
> > > > > > > > > > cat custom_ima_policy > /sys/kernel/security/ima/policy
> > > > > > > > > > 
> > > > > > > > > > or by writing the absolute path of the file containing the
> > > > > > > > > > custom
> > > > > > > > > > policy:
> > > > > > > > > > 
> > > > > > > > > > echo /path/of/custom_ima_policy >
> > > > > > > > > > /sys/kernel/security/ima/policy
> > > > > > > > > > 
> > > > > > > > > > In these cases, file signature can be necessary to load the
> > > > > > > > > > policy
> > > > > > > > > > (func=POLICY_CHECK). Custom policy can also be set at
> > > > > > > > > > runtime by
> > > > > > > > > > directly
> > > > > > > > > > writing the policy stream on the ima policy file:
> > > > > > > > > > 
> > > > > > > > > > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > > > >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> > > > > > > > > >      > /sys/kernel/security/ima/policy
> > > > > > > > > > 
> > > > > > > > > > In this case, there is no mechanism to verify the integrity
> > > > > > > > > > of
> > > > > > > > > > the
> > > > > > > > > > new
> > > > > > > > > > policy.
> > > > > > > > > > 
> > > > > > > > > > Add a new entry in the ima measurements list containing the
> > > > > > > > > > ascii
> > > > > > > > > > custom
> > > > > > > > > > ima policy buffer when not verified at load time.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > > > > > > > > 
> > > > > > > > > Hi Enrico,
> > > > > > > > 
> > > > > > > > Hi Mimi,
> > > > > > > > 
> > > > > > > > thank you for the quick response.
> > > > > > > > 
> > > > > > > > > This patch set hard codes measuring the initial custom IMA
> > > > > > > > > policy
> > > > > > > > > rules
> > > > > > > > > that
> > > > > > > > > replace the builtin policies specified on the boot command
> > > > > > > > > line. 
> > > > > > > > > IMA
> > > > > > > > > shouldn't hard code policy.
> > > > > > > > 
> > > > > > > > My first approach was to define a new critical-data record, 
> > > > > > 
> > > > > > Hi Mimi,
> > > > > > 
> > > > > > > Hopefully the new critical-data will be of the entire IMA policy.
> > > > > > 
> > > > > > yes, absolutely.
> > > > > > 
> > > > > > > > but performing the
> > > > > > > > measurement after the custom policy becomes effective, the
> > > > > > > > measurement
> > > > > > > > could
> > > > > > > > be
> > > > > > > > bypassed omitting func=CRITICAL_DATA in the custom policy.
> > > > > > > > 
> > > > > > > > > I'm not quite sure why you're differentiating between
> > > > > > > > > measuring the initial and subsequent custom IMA policy
> > > > > > > > > rules.  
> > > > > > > > 
> > > > > > > > My intention is to measure the first direct write (line by line)
> > > > > > > > on
> > > > > > > > the
> > > > > > > > policy
> > > > > > > > file, without loading the initial custom policy from a file.
> > > > > > > > This
> > > > > > > > case,
> > > > > > > > if
> > > > > > > > I'm
> > > > > > > > not wrong, is not covered by func=POLICY_CHECK.
> > > > > > > 
> > > > > > > When secure boot is enabled, the arch specific policy rules
> > > > > > > require
> > > > > > > the
> > > > > > > IMA
> > > > > > > policy to be signed.  Without secure boot enabled, you're correct.
> > > > > > > The
> > > > > > > custom
> > > > > > > policy rules may directly be loaded without being measured.
> > > > > > > 
> > > > > > > Why only measure "the first direct write"?  Additional custom
> > > > > > > policy
> > > > > > > rules
> > > > > > > may
> > > > > > > be directly appended without being measured.
> > > > > > 
> > > > > > Yes, you right. The aim was to measure (at least) the first one,
> > > > > > because
> > > > > > it
> > > > > > substitutes the boot policy, but if you are ok with adding a
> > > > > > critical-
> > > > > > data
> > > > > > record, it would be definitely better.
> > > > > 
> > > > > Hi Enrico
> > > > > 
> > > > > in addition to what Mimi suggested, I also like to idea that the
> > > > > POLICY_CHECK hook catches the direct policy loading. That would mean
> > > > > that those updates would be seen if the 'tcb' IMA policy is selected.
> > > > 
> > > > Hi Roberto,
> > > > 
> > > > in this case, wouldn't be used the current template? Wouldn't be better
> > > > to
> > > > use
> > > > the ima-buf in order to include the textual policy representation?
> > > 
> > > Hi Enrico
> > > 
> > > I would use the current template, I don't find any particular issues
> > > for it. Sure, we don't have a file to measure but there are other cases
> > > where in process_measurement() we measure a buffer instead of a file
> > > (when it is called by ima_post_read_file()).
> > > 
> > > We can have both critical data and POLICY_CHECK measurement.
> 
> CRITICAL_DATA and POLICY_CHECK are separate hooks.
> 
> They can do measurement differently, CRITICAL_DATA can be for measuring
> the current state of the full policy, while POLICY_CHECK could be for
> policies sent to the kernel.

Hi Roberto,

ok, got it! Thanks for the clarification.

Enrico

> > Hi Roberto,
> > 
> > sorry, I didn't get this point. What do you mean?
> > 
> > > > In addition, there would be a new record for each line of the input
> > > > buffer,
> > > > and
> > > > measuring the input buffer would produce different measurements for the
> > > > same
> > > > resulting policy entry, because different or multiple separators can be
> > > > used.
> > > > 
> > > > I opted to perform the measurement in ima_release_policy() because is
> > > > where
> > > > the
> > > > new policy becomes effective after ima_update_policy() and can be done a
> > > > single
> > > > measurement of the new running policy.
> > > 
> > > I would simply measure what is passed to ima_write_policy() regardless
> > > of whether the policy will be accepted or not. This is more in line
> > > with the trusted computing paradigm of measure & load. If potentially
> > > there is a bug in the policy code, measuring the policy before with a
> > > vulnerable kernel would allow you to see the measurement. After, it
> > > depends on the seriousness of the vulnerability.
> > 
> > Ok perfect, I get your point. Thank you for the explanation.
> 
> Welcome.
> 
> Roberto
> 
> > Enrico
> > 
> > > Roberto
> > > 
> > > > The measurement could be done a bit earlier, working on ima_policy_rules
> > > > and
> > > > ima_temp_rules (which basically contains the input buffer) before the
> > > > splicing,
> > > > so it would be considered the current policy and not the new one. In
> > > > this
> > > > case,
> > > > it would work also when ima_policy=tcb is set, and it could be called
> > > > process_buffer_measurement() with POLICY_CHECK, to get a record with the
> > > > entire
> > > > IMA policy.
> > > > What do you think about it?
> > > > 
> > > > BR,
> > > > 
> > > > Enrico
> > > > 
> > > > > I would have recommended to try to add a process_measurement() call in
> > > > > ima_write_policy(), where the buffer to be processed is.
> > > > > 
> > > > > However, I guess you need to have a valid file descriptor in order to
> > > > > use that function (maybe an anonymous inode?).
> > > > > ima_collect_measurement() should be already able to handle buffers,
> > > > > passed by ima_post_read_file().
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > Roberto
> > > > > 
> > > > > > Thank you,
> > > > > > 
> > > > > > Enrico
> > > > > > 
> > > > > > > > 
> > > > > > > > > Consider defining a new critical-data record to measure the
> > > > > > > > > current
> > > > > > > > > IMA
> > > > > > > > > policy
> > > > > > > > > rules.  Also consider including the new critical-data rule in
> > > > > > > > > the
> > > > > > > > > arch
> > > > > > > > > specific policy rules.
> > > > > > > > 
> > > > > > > > I think that your suggestion, to add the critical-data rule in
> > > > > > > > the
> > > > > > > > arch
> > > > > > > > policy
> > > > > > > > rules, solves the problems of bypassing the measurement and hard
> > > > > > > > coding
> > > > > > > > policy.
> > > > > > > > 
> > > > > > > > Thank you very much for your feedback.
> > > > > > > 
> > > > > > > You're welcome.
> > > > > > > 
> > > > > > > Mimi
> > > > > > > > 
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

[RFC][PATCH v2 0/2] ima: measure write on securityfs policy file

[Enrico Bravi]

This series aims to introduce integrity measurements when the IMA policy is
written on the securityfs file.
In particular, when a signed policy is not mandatory, it can be written
directly on the securityfs file. This allows to override the boot policy
at the first write, and append new policy rules at the subsequent writes (if
CONFIG_IMA_WRITE_POLICY=y). In this case new policy can be loaded
without being measured.

The patch #1 introduces a new critical-data record for the newly loaded
policy. The measurement is performed over the textual representation of the
new policy once it becomes effective (after ima_update_policy()). As
suggested by Mimi, the new critical-data rule is added to the arch
specific policy rules.

The patch #2, following what suggested by Roberto, measures the input
buffer sent to the securityfs policy file, regardless of whether the new
policy will be accepted or not. This is done by calling
process_measurement() in order to catch it when 'measure
func=POLICY_CHECK' is enabled (e.g., ima_policy=tcb).

Changes in v2:
 - Set a new critical-data rule for measuring the loaded IMA policy.
 - Add the new critical-data rule to the specific arch policy rules.
 - Add patch #2 for measuring the input buffer sent to the securityfs
   policy file.

Enrico Bravi (2):
  ima: measure loaded policy after write on securityfs policy file
  ima: measure buffer sent to securityfs policy file

 security/integrity/ima/ima.h        |  2 +
 security/integrity/ima/ima_efi.c    |  1 +
 security/integrity/ima/ima_fs.c     |  2 +
 security/integrity/ima/ima_main.c   | 38 +++++++++++++++++
 security/integrity/ima/ima_policy.c | 63 ++++++++++++++++++++++++++++-
 5 files changed, 104 insertions(+), 2 deletions(-)


base-commit: 7d0a66e4bb9081d75c82ec4957c50034cb0ea449
-- 
2.52.0

[RFC][PATCH v2 1/2] ima: measure loaded policy after write on securityfs policy file

[Enrico Bravi]

IMA policy can be written multiple times in the securityfs policy file
at runtime if CONFIG_IMA_WRITE_POLICY=y. When IMA_APPRAISE_POLICY is
required, the policy needs to be signed to be loaded, writing the absolute
path of the file containing the new policy:

echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy

When this is not required, policy can be written directly, rule by rule:

echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
        "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
     > /sys/kernel/security/ima/policy

In this case, a new policy can be loaded without being measured or
appraised.

Add a new critical data record to measure the textual policy
representation when it becomes effective.

Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
---
 security/integrity/ima/ima.h        |  1 +
 security/integrity/ima/ima_efi.c    |  1 +
 security/integrity/ima/ima_fs.c     |  1 +
 security/integrity/ima/ima_policy.c | 63 ++++++++++++++++++++++++++++-
 4 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index e3d71d8d56e3..ca7b96663623 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -425,6 +425,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
 void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
 void ima_policy_stop(struct seq_file *m, void *v);
 int ima_policy_show(struct seq_file *m, void *v);
+void ima_measure_loaded_policy(void);
 
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE	0x01
diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
index 138029bfcce1..199c42d0f8b3 100644
--- a/security/integrity/ima/ima_efi.c
+++ b/security/integrity/ima/ima_efi.c
@@ -62,6 +62,7 @@ static const char * const sb_arch_rules[] = {
 	"appraise func=POLICY_CHECK appraise_type=imasig",
 #endif
 	"measure func=MODULE_CHECK",
+	"measure func=CRITICAL_DATA label=ima_policy",
 	NULL
 };
 
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 87045b09f120..89946d803d44 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -476,6 +476,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
 	}
 
 	ima_update_policy();
+	ima_measure_loaded_policy();
 #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
 	securityfs_remove(file->f_path.dentry);
 #elif defined(CONFIG_IMA_WRITE_POLICY)
diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
index 128fab897930..956063748008 100644
--- a/security/integrity/ima/ima_policy.c
+++ b/security/integrity/ima/ima_policy.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/rculist.h>
 #include <linux/seq_file.h>
+#include <linux/vmalloc.h>
 #include <linux/ima.h>
 
 #include "ima.h"
@@ -1983,7 +1984,6 @@ const char *const func_tokens[] = {
 	__ima_hooks(__ima_hook_stringify)
 };
 
-#ifdef	CONFIG_IMA_READ_POLICY
 enum {
 	mask_exec = 0, mask_write, mask_read, mask_append
 };
@@ -2277,7 +2277,6 @@ int ima_policy_show(struct seq_file *m, void *v)
 	seq_puts(m, "\n");
 	return 0;
 }
-#endif	/* CONFIG_IMA_READ_POLICY */
 
 #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
 /*
@@ -2334,3 +2333,63 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
 	return found;
 }
 #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
+
+static size_t ima_policy_text_len(void)
+{
+	struct list_head *ima_rules_tmp;
+	struct ima_rule_entry *entry;
+	struct seq_file file;
+	size_t size = 0;
+	char rule[255];
+
+	file.buf = rule;
+	file.read_pos = 0;
+	file.size = 255;
+	file.count = 0;
+
+	rcu_read_lock();
+	ima_rules_tmp = rcu_dereference(ima_rules);
+	list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
+		ima_policy_show(&file, entry);
+		size += file.count;
+		file.count = 0;
+	}
+	rcu_read_unlock();
+
+	return size;
+}
+
+void ima_measure_loaded_policy(void)
+{
+	const char *event_name = "ima_policy_loaded";
+	const char *op = "measure_loaded_ima_policy";
+	const char *audit_cause = "ENOMEM";
+	struct ima_rule_entry *rule_entry;
+	struct list_head *ima_rules_tmp;
+	struct seq_file file;
+	int result = -ENOMEM;
+	size_t file_len = ima_policy_text_len();
+
+	file.buf = vmalloc(file_len);
+	if (!file.buf) {
+		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name,
+				    op, audit_cause, result, 1);
+		return;
+	}
+
+	file.read_pos = 0;
+	file.size = file_len;
+	file.count = 0;
+
+	rcu_read_lock();
+	ima_rules_tmp = rcu_dereference(ima_rules);
+	list_for_each_entry_rcu(rule_entry, ima_rules_tmp, list) {
+		ima_policy_show(&file, rule_entry);
+	}
+	rcu_read_unlock();
+
+	ima_measure_critical_data("ima_policy", event_name, file.buf,
+				  file.count, false, NULL, 0);
+
+	vfree(file.buf);
+}
-- 
2.52.0

Re: [RFC][PATCH v2 1/2] ima: measure loaded policy after write on securityfs policy file

[Mimi Zohar]

On Tue, 2025-12-16 at 17:56 +0100, Enrico Bravi wrote:
> IMA policy can be written multiple times in the securityfs policy file
> at runtime if CONFIG_IMA_WRITE_POLICY=y. When IMA_APPRAISE_POLICY is
> required, the policy needs to be signed to be loaded, writing the absolute
> path of the file containing the new policy:
> 
> echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> 
> When this is not required, policy can be written directly, rule by rule:
> 
> echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
>         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
>      > /sys/kernel/security/ima/policy
> 
> In this case, a new policy can be loaded without being measured or
> appraised.
> 
> Add a new critical data record to measure the textual policy
> representation when it becomes effective.

The IMA policy could be really large.  Do we really want to include all the
policy rules in the template data?  The other option would be to include a hash
of the policy rules, in lieu of the policy rules themselves.

Please include directions for verifying the critical-data (e.g. using xxd).

> 
> Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> ---
>  security/integrity/ima/ima.h        |  1 +
>  security/integrity/ima/ima_efi.c    |  1 +
>  security/integrity/ima/ima_fs.c     |  1 +
>  security/integrity/ima/ima_policy.c | 63 ++++++++++++++++++++++++++++-
>  4 files changed, 64 insertions(+), 2 deletions(-)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index e3d71d8d56e3..ca7b96663623 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -425,6 +425,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
>  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>  void ima_policy_stop(struct seq_file *m, void *v);
>  int ima_policy_show(struct seq_file *m, void *v);
> +void ima_measure_loaded_policy(void);
>  
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE	0x01
> diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
> index 138029bfcce1..199c42d0f8b3 100644
> --- a/security/integrity/ima/ima_efi.c
> +++ b/security/integrity/ima/ima_efi.c
> @@ -62,6 +62,7 @@ static const char * const sb_arch_rules[] = {
>  	"appraise func=POLICY_CHECK appraise_type=imasig",
>  #endif
>  	"measure func=MODULE_CHECK",
> +	"measure func=CRITICAL_DATA label=ima_policy",

With this rule, the policy will always be measured, even when loading a signed
policy file.  It that necessary?

>  	NULL
>  };
>  
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 87045b09f120..89946d803d44 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -476,6 +476,7 @@ static int ima_release_policy(struct inode *inode, struct file *file)
>  	}
>  
>  	ima_update_policy();
> +	ima_measure_loaded_policy();
>  #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
>  	securityfs_remove(file->f_path.dentry);
>  #elif defined(CONFIG_IMA_WRITE_POLICY)
> diff --git a/security/integrity/ima/ima_policy.c b/security/integrity/ima/ima_policy.c
> index 128fab897930..956063748008 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -17,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/rculist.h>
>  #include <linux/seq_file.h>
> +#include <linux/vmalloc.h>
>  #include <linux/ima.h>
>  
>  #include "ima.h"
> @@ -1983,7 +1984,6 @@ const char *const func_tokens[] = {
>  	__ima_hooks(__ima_hook_stringify)
>  };
>  
> -#ifdef	CONFIG_IMA_READ_POLICY
>  enum {
>  	mask_exec = 0, mask_write, mask_read, mask_append
>  };
> @@ -2277,7 +2277,6 @@ int ima_policy_show(struct seq_file *m, void *v)
>  	seq_puts(m, "\n");
>  	return 0;
>  }
> -#endif	/* CONFIG_IMA_READ_POLICY */
>  
>  #if defined(CONFIG_IMA_APPRAISE) && defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
>  /*
> @@ -2334,3 +2333,63 @@ bool ima_appraise_signature(enum kernel_read_file_id id)
>  	return found;
>  }
>  #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
> +
> +static size_t ima_policy_text_len(void)
> +{
> +	struct list_head *ima_rules_tmp;
> +	struct ima_rule_entry *entry;
> +	struct seq_file file;
> +	size_t size = 0;
> +	char rule[255];
> +
> +	file.buf = rule;
> +	file.read_pos = 0;
> +	file.size = 255;
> +	file.count = 0;
> +
> +	rcu_read_lock();
> +	ima_rules_tmp = rcu_dereference(ima_rules);
> +	list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> +		ima_policy_show(&file, entry);
> +		size += file.count;
> +		file.count = 0;
> +	}
> +	rcu_read_unlock();
> +
> +	return size;
> +}
> +
> +void ima_measure_loaded_policy(void)
> +{
> +	const char *event_name = "ima_policy_loaded";
> +	const char *op = "measure_loaded_ima_policy";
> +	const char *audit_cause = "ENOMEM";
> +	struct ima_rule_entry *rule_entry;
> +	struct list_head *ima_rules_tmp;
> +	struct seq_file file;
> +	int result = -ENOMEM;
> +	size_t file_len = ima_policy_text_len();

Normally a function is defined to prevent code duplication or for readability. 
In this case, it doesn't achieve either.

Add a comment here, something like:
/* calculate IMA policy rules memory size */

> +
> +	file.buf = vmalloc(file_len);
> +	if (!file.buf) {
> +		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name,
> +				    op, audit_cause, result, 1);
> +		return;
> +	}
> 
> 

And another comment below, something like:
/* copy IMA policy rules to a buffer */

> +
> +	file.read_pos = 0;
> +	file.size = file_len;
> +	file.count = 0;
> +
> +	rcu_read_lock();
> +	ima_rules_tmp = rcu_dereference(ima_rules);
> +	list_for_each_entry_rcu(rule_entry, ima_rules_tmp, list) {
> +		ima_policy_show(&file, rule_entry);
> +	}
> +	rcu_read_unlock();
> +
> +	ima_measure_critical_data("ima_policy", event_name, file.buf,
> +				  file.count, false, NULL, 0);
> +
> +	vfree(file.buf);
> +}

Thanks,

Mimi

Re: [RFC][PATCH v2 1/2] ima: measure loaded policy after write on securityfs policy file

[Enrico Bravi]

On Fri, 2025-12-19 at 14:13 -0500, Mimi Zohar wrote:
> On Tue, 2025-12-16 at 17:56 +0100, Enrico Bravi wrote:
> > IMA policy can be written multiple times in the securityfs policy file
> > at runtime if CONFIG_IMA_WRITE_POLICY=y. When IMA_APPRAISE_POLICY is
> > required, the policy needs to be signed to be loaded, writing the absolute
> > path of the file containing the new policy:
> > 
> > echo /path/of/custom_ima_policy > /sys/kernel/security/ima/policy
> > 
> > When this is not required, policy can be written directly, rule by rule:
> > 
> > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> >      > /sys/kernel/security/ima/policy
> > 
> > In this case, a new policy can be loaded without being measured or
> > appraised.
> > 
> > Add a new critical data record to measure the textual policy
> > representation when it becomes effective.

Hi Mimi,

sorry for the delay in answering you.

> The IMA policy could be really large.  Do we really want to include all the
> policy rules in the template data?  The other option would be to include a
> hash of the policy rules, in lieu of the policy rules themselves.

I thought to include all the policy rules because the policy can be defined
using the same rules in a different order. For large policy this would mean that
a verifier should have several hash values for the same policy.

> Please include directions for verifying the critical-data (e.g. using xxd).

Yes, sure.

> > 
> > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > ---
> >  security/integrity/ima/ima.h        |  1 +
> >  security/integrity/ima/ima_efi.c    |  1 +
> >  security/integrity/ima/ima_fs.c     |  1 +
> >  security/integrity/ima/ima_policy.c | 63 ++++++++++++++++++++++++++++-
> >  4 files changed, 64 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index e3d71d8d56e3..ca7b96663623 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -425,6 +425,7 @@ void *ima_policy_start(struct seq_file *m, loff_t *pos);
> >  void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
> >  void ima_policy_stop(struct seq_file *m, void *v);
> >  int ima_policy_show(struct seq_file *m, void *v);
> > +void ima_measure_loaded_policy(void);
> >  
> >  /* Appraise integrity measurements */
> >  #define IMA_APPRAISE_ENFORCE 0x01
> > diff --git a/security/integrity/ima/ima_efi.c
> > b/security/integrity/ima/ima_efi.c
> > index 138029bfcce1..199c42d0f8b3 100644
> > --- a/security/integrity/ima/ima_efi.c
> > +++ b/security/integrity/ima/ima_efi.c
> > @@ -62,6 +62,7 @@ static const char * const sb_arch_rules[] = {
> >   "appraise func=POLICY_CHECK appraise_type=imasig",
> >  #endif
> >   "measure func=MODULE_CHECK",
> > + "measure func=CRITICAL_DATA label=ima_policy",
> 
> With this rule, the policy will always be measured, even when loading a signed
> policy file.  It that necessary?

No you're right. I can include it only in the case the policy is not apprised.

> >   NULL
> >  };
> >  
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index 87045b09f120..89946d803d44 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -476,6 +476,7 @@ static int ima_release_policy(struct inode *inode,
> > struct file *file)
> >   }
> >  
> >   ima_update_policy();
> > + ima_measure_loaded_policy();
> >  #if !defined(CONFIG_IMA_WRITE_POLICY) && !defined(CONFIG_IMA_READ_POLICY)
> >   securityfs_remove(file->f_path.dentry);
> >  #elif defined(CONFIG_IMA_WRITE_POLICY)
> > diff --git a/security/integrity/ima/ima_policy.c
> > b/security/integrity/ima/ima_policy.c
> > index 128fab897930..956063748008 100644
> > --- a/security/integrity/ima/ima_policy.c
> > +++ b/security/integrity/ima/ima_policy.c
> > @@ -17,6 +17,7 @@
> >  #include <linux/slab.h>
> >  #include <linux/rculist.h>
> >  #include <linux/seq_file.h>
> > +#include <linux/vmalloc.h>
> >  #include <linux/ima.h>
> >  
> >  #include "ima.h"
> > @@ -1983,7 +1984,6 @@ const char *const func_tokens[] = {
> >   __ima_hooks(__ima_hook_stringify)
> >  };
> >  
> > -#ifdef CONFIG_IMA_READ_POLICY
> >  enum {
> >   mask_exec = 0, mask_write, mask_read, mask_append
> >  };
> > @@ -2277,7 +2277,6 @@ int ima_policy_show(struct seq_file *m, void *v)
> >   seq_puts(m, "\n");
> >   return 0;
> >  }
> > -#endif /* CONFIG_IMA_READ_POLICY */
> >  
> >  #if defined(CONFIG_IMA_APPRAISE) &&
> > defined(CONFIG_INTEGRITY_TRUSTED_KEYRING)
> >  /*
> > @@ -2334,3 +2333,63 @@ bool ima_appraise_signature(enum kernel_read_file_id
> > id)
> >   return found;
> >  }
> >  #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
> > +
> > +static size_t ima_policy_text_len(void)
> > +{
> > + struct list_head *ima_rules_tmp;
> > + struct ima_rule_entry *entry;
> > + struct seq_file file;
> > + size_t size = 0;
> > + char rule[255];
> > +
> > + file.buf = rule;
> > + file.read_pos = 0;
> > + file.size = 255;
> > + file.count = 0;
> > +
> > + rcu_read_lock();
> > + ima_rules_tmp = rcu_dereference(ima_rules);
> > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) {
> > + ima_policy_show(&file, entry);
> > + size += file.count;
> > + file.count = 0;
> > + }
> > + rcu_read_unlock();
> > +
> > + return size;
> > +}
> > +
> > +void ima_measure_loaded_policy(void)
> > +{
> > + const char *event_name = "ima_policy_loaded";
> > + const char *op = "measure_loaded_ima_policy";
> > + const char *audit_cause = "ENOMEM";
> > + struct ima_rule_entry *rule_entry;
> > + struct list_head *ima_rules_tmp;
> > + struct seq_file file;
> > + int result = -ENOMEM;
> > + size_t file_len = ima_policy_text_len();
> 
> Normally a function is defined to prevent code duplication or for readability.
> In this case, it doesn't achieve either.

Ok, yes you're right. I'll move the ima_policy_text_len() code in
ima_measure_loaded_policy().

Thank you very much for your feedback.

Enrico

> Add a comment here, something like:
> /* calculate IMA policy rules memory size */
> 
> > +
> > + file.buf = vmalloc(file_len);
> > + if (!file.buf) {
> > + integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name,
> > +     op, audit_cause, result, 1);
> > + return;
> > + }
> > 
> > 
> 
> And another comment below, something like:
> /* copy IMA policy rules to a buffer */
> 
> > +
> > + file.read_pos = 0;
> > + file.size = file_len;
> > + file.count = 0;
> > +
> > + rcu_read_lock();
> > + ima_rules_tmp = rcu_dereference(ima_rules);
> > + list_for_each_entry_rcu(rule_entry, ima_rules_tmp, list) {
> > + ima_policy_show(&file, rule_entry);
> > + }
> > + rcu_read_unlock();
> > +
> > + ima_measure_critical_data("ima_policy", event_name, file.buf,
> > +   file.count, false, NULL, 0);
> > +
> > + vfree(file.buf);
> > +}
> 
> Thanks,
> 
> Mimi

[RFC][PATCH v2 2/2] ima: measure buffer sent to securityfs policy file

[Enrico Bravi]

When signed a policy is not mandatory, it is possile to write the IMA
policy directly on the corresponding securityfs file:

echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
        "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
     > /sys/kernel/security/ima/policy

Add input buffer measurement that can be caught when 'measure
func=POLICY_CHECK' is enabled (e.g., ima_policy=tcb).

Suggested-by: Roberto Sassu <roberto.sassu@huawei.com>
Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
---
 security/integrity/ima/ima.h      |  1 +
 security/integrity/ima/ima_fs.c   |  1 +
 security/integrity/ima/ima_main.c | 38 +++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)

diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
index ca7b96663623..3b00c298355b 100644
--- a/security/integrity/ima/ima.h
+++ b/security/integrity/ima/ima.h
@@ -426,6 +426,7 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
 void ima_policy_stop(struct seq_file *m, void *v);
 int ima_policy_show(struct seq_file *m, void *v);
 void ima_measure_loaded_policy(void);
+int ima_measure_policy_write(char *buf, size_t size);
 
 /* Appraise integrity measurements */
 #define IMA_APPRAISE_ENFORCE	0x01
diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
index 89946d803d44..f1a5edd060ad 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -362,6 +362,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
 				    1, 0);
 		result = -EACCES;
 	} else {
+		ima_measure_policy_write(data, datalen);
 		result = ima_parse_add_rule(data);
 	}
 	mutex_unlock(&ima_write_mutex);
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index cdd225f65a62..6a8ad4714881 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -28,6 +28,7 @@
 #include <linux/iversion.h>
 #include <linux/evm.h>
 #include <linux/crash_dump.h>
+#include <linux/shmem_fs.h>
 
 #include "ima.h"
 
@@ -986,6 +987,43 @@ static int ima_post_load_data(char *buf, loff_t size,
 	return 0;
 }
 
+/**
+ * ima_measure_policy_write - Measure the policy write buffer
+ * @buf: pointer to the buffer containing the policy write data
+ * @size: size of the buffer
+ *
+ * Measure the buffer sent to the IMA policy securityfs file.
+ *
+ * Return 0 on success, a negative value otherwise.
+ */
+int ima_measure_policy_write(char *buf, size_t size)
+{
+	static const char op[] = "measure_ima_policy_write";
+	const char *file_name = "ima_write_policy_buffer";
+	static char *audit_cause = "ENOMEM";
+	struct file *policy_file = NULL;
+	struct lsm_prop prop;
+	int ret = 0;
+
+	policy_file = shmem_kernel_file_setup(file_name, 0, 0);
+	if (IS_ERR(policy_file)) {
+		ret = PTR_ERR(policy_file);
+		audit_cause = "alloc_file";
+		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, "ima_policy_write",
+				    op, audit_cause, ret, 1);
+		goto out;
+	}
+
+	security_current_getlsmprop_subj(&prop);
+
+	ret = process_measurement(policy_file, current_cred(), &prop, buf, size,
+				  MAY_READ, POLICY_CHECK);
+	fput(policy_file);
+
+out:
+	return ret;
+}
+
 /**
  * process_buffer_measurement - Measure the buffer or the buffer data hash
  * @idmap: idmap of the mount the inode was found from
-- 
2.52.0

Re: [RFC][PATCH v2 2/2] ima: measure buffer sent to securityfs policy file

[Mimi Zohar]

Hi Enrico,

On Tue, 2025-12-16 at 17:56 +0100, Enrico Bravi wrote:
> When signed a policy is not mandatory, it is possile to write the IMA
> policy directly on the corresponding securityfs file:
> 
> echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
>         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
>      > /sys/kernel/security/ima/policy
> 
> Add input buffer measurement that can be caught when 'measure
> func=POLICY_CHECK' is enabled (e.g., ima_policy=tcb).
> 
> Suggested-by: Roberto Sassu <roberto.sassu@huawei.com>
> Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> ---
>  security/integrity/ima/ima.h      |  1 +
>  security/integrity/ima/ima_fs.c   |  1 +
>  security/integrity/ima/ima_main.c | 38 +++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
> 
> diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> index ca7b96663623..3b00c298355b 100644
> --- a/security/integrity/ima/ima.h
> +++ b/security/integrity/ima/ima.h
> @@ -426,6 +426,7 @@ void *ima_policy_next(struct seq_file *m, void *v, loff_t *pos);
>  void ima_policy_stop(struct seq_file *m, void *v);
>  int ima_policy_show(struct seq_file *m, void *v);
>  void ima_measure_loaded_policy(void);
> +int ima_measure_policy_write(char *buf, size_t size);
>  
>  /* Appraise integrity measurements */
>  #define IMA_APPRAISE_ENFORCE	0x01
> diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> index 89946d803d44..f1a5edd060ad 100644
> --- a/security/integrity/ima/ima_fs.c
> +++ b/security/integrity/ima/ima_fs.c
> @@ -362,6 +362,7 @@ static ssize_t ima_write_policy(struct file *file, const char __user *buf,
>  				    1, 0);
>  		result = -EACCES;
>  	} else {
> +		ima_measure_policy_write(data, datalen);
>  		result = ima_parse_add_rule(data);
>  	}
>  	mutex_unlock(&ima_write_mutex);
> diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
> index cdd225f65a62..6a8ad4714881 100644
> --- a/security/integrity/ima/ima_main.c
> +++ b/security/integrity/ima/ima_main.c
> @@ -28,6 +28,7 @@
>  #include <linux/iversion.h>
>  #include <linux/evm.h>
>  #include <linux/crash_dump.h>
> +#include <linux/shmem_fs.h>
>  
>  #include "ima.h"
>  
> @@ -986,6 +987,43 @@ static int ima_post_load_data(char *buf, loff_t size,
>  	return 0;
>  }
>  
> +/**
> + * ima_measure_policy_write - Measure the policy write buffer
> + * @buf: pointer to the buffer containing the policy write data
> + * @size: size of the buffer
> + *
> + * Measure the buffer sent to the IMA policy securityfs file.
> + *
> + * Return 0 on success, a negative value otherwise.
> + */
> +int ima_measure_policy_write(char *buf, size_t size0
> +{
> +	static const char op[] = "measure_ima_policy_write";
> +	const char *file_name = "ima_write_policy_buffer";
> +	static char *audit_cause = "ENOMEM";
> +	struct file *policy_file = NULL;
> +	struct lsm_prop prop;
> +	int ret = 0;
> +
> +	policy_file = shmem_kernel_file_setup(file_name, 0, 0);
> +	if (IS_ERR(policy_file)) {
> +		ret = PTR_ERR(policy_file);
> +		audit_cause = "alloc_file";
> +		integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, "ima_policy_write",
> +				    op, audit_cause, ret, 1);
> +		goto out;
> +	}
> +
> +	security_current_getlsmprop_subj(&prop);
> +
> +	ret = process_measurement(policy_file, current_cred(), &prop, buf, size,
> +				  MAY_READ, POLICY_CHECK);

The purpose of this patch is to measure IMA policy rules as they're written to
the <securityfs> IMA policy file, based on the IMA "measure func=POLICY_CHECK"
policy rule.

Like critical data, it should be calling process_buffer_measurement(), not
process_measurement().

The functions ima_match_rules() and ima_match_rule_data() need to be updated to
support POLICY_CHECK.

This function naming is off and should be renamed to ima_measure_policy_buf().

Please update the patch description accordingly.

Mimi

> +	fput(policy_file);
> +
> +out:
> +	return ret;
> +}
> +
>  /**
>   * process_buffer_measurement - Measure the buffer or the buffer data hash
>   * @idmap: idmap of the mount the inode was found from

Re: [RFC][PATCH v2 2/2] ima: measure buffer sent to securityfs policy file

[Enrico Bravi]

On Mon, 2025-12-22 at 09:01 -0500, Mimi Zohar wrote:
> Hi Enrico,
> 
> On Tue, 2025-12-16 at 17:56 +0100, Enrico Bravi wrote:
> > When signed a policy is not mandatory, it is possile to write the IMA
> > policy directly on the corresponding securityfs file:
> > 
> > echo -e "measure func=BPRM_CHECK mask=MAY_EXEC\n" \
> >         "audit func=BPRM_CHECK mask=MAY_EXEC\n" \
> >      > /sys/kernel/security/ima/policy
> > 
> > Add input buffer measurement that can be caught when 'measure
> > func=POLICY_CHECK' is enabled (e.g., ima_policy=tcb).
> > 
> > Suggested-by: Roberto Sassu <roberto.sassu@huawei.com>
> > Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> > ---
> >  security/integrity/ima/ima.h      |  1 +
> >  security/integrity/ima/ima_fs.c   |  1 +
> >  security/integrity/ima/ima_main.c | 38 +++++++++++++++++++++++++++++++
> >  3 files changed, 40 insertions(+)
> > 
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index ca7b96663623..3b00c298355b 100644
> > --- a/security/integrity/ima/ima.h
> > +++ b/security/integrity/ima/ima.h
> > @@ -426,6 +426,7 @@ void *ima_policy_next(struct seq_file *m, void *v,
> > loff_t *pos);
> >  void ima_policy_stop(struct seq_file *m, void *v);
> >  int ima_policy_show(struct seq_file *m, void *v);
> >  void ima_measure_loaded_policy(void);
> > +int ima_measure_policy_write(char *buf, size_t size);
> >  
> >  /* Appraise integrity measurements */
> >  #define IMA_APPRAISE_ENFORCE 0x01
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index 89946d803d44..f1a5edd060ad 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -362,6 +362,7 @@ static ssize_t ima_write_policy(struct file *file, const
> > char __user *buf,
> >       1, 0);
> >   result = -EACCES;
> >   } else {
> > + ima_measure_policy_write(data, datalen);
> >   result = ima_parse_add_rule(data);
> >   }
> >   mutex_unlock(&ima_write_mutex);
> > diff --git a/security/integrity/ima/ima_main.c
> > b/security/integrity/ima/ima_main.c
> > index cdd225f65a62..6a8ad4714881 100644
> > --- a/security/integrity/ima/ima_main.c
> > +++ b/security/integrity/ima/ima_main.c
> > @@ -28,6 +28,7 @@
> >  #include <linux/iversion.h>
> >  #include <linux/evm.h>
> >  #include <linux/crash_dump.h>
> > +#include <linux/shmem_fs.h>
> >  
> >  #include "ima.h"
> >  
> > @@ -986,6 +987,43 @@ static int ima_post_load_data(char *buf, loff_t size,
> >   return 0;
> >  }
> >  
> > +/**
> > + * ima_measure_policy_write - Measure the policy write buffer
> > + * @buf: pointer to the buffer containing the policy write data
> > + * @size: size of the buffer
> > + *
> > + * Measure the buffer sent to the IMA policy securityfs file.
> > + *
> > + * Return 0 on success, a negative value otherwise.
> > + */
> > +int ima_measure_policy_write(char *buf, size_t size0
> > +{
> > + static const char op[] = "measure_ima_policy_write";
> > + const char *file_name = "ima_write_policy_buffer";
> > + static char *audit_cause = "ENOMEM";
> > + struct file *policy_file = NULL;
> > + struct lsm_prop prop;
> > + int ret = 0;
> > +
> > + policy_file = shmem_kernel_file_setup(file_name, 0, 0);
> > + if (IS_ERR(policy_file)) {
> > + ret = PTR_ERR(policy_file);
> > + audit_cause = "alloc_file";
> > + integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, "ima_policy_write",
> > +     op, audit_cause, ret, 1);
> > + goto out;
> > + }
> > +
> > + security_current_getlsmprop_subj(&prop);
> > +
> > + ret = process_measurement(policy_file, current_cred(), &prop, buf, size,
> > +   MAY_READ, POLICY_CHECK);
> 
> The purpose of this patch is to measure IMA policy rules as they're written to
> the <securityfs> IMA policy file, based on the IMA "measure func=POLICY_CHECK"
> policy rule.
> 
> Like critical data, it should be calling process_buffer_measurement(), not
> process_measurement().
> 
> The functions ima_match_rules() and ima_match_rule_data() need to be updated
> to support POLICY_CHECK.

Hi Mimi,

ok sure, I can switch to process_buffer_measurement().

> This function naming is off and should be renamed to ima_measure_policy_buf().
> 
> Please update the patch description accordingly.

Will do.

Thanks again for your feedback.

Enrico

> Mimi
> 
> > + fput(policy_file);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> >  /**
> >   * process_buffer_measurement - Measure the buffer or the buffer data hash
> >   * @idmap: idmap of the mount the inode was found from