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

Hi Mimi,

On Wed, 2026-06-10 at 10:32 -0400, Mimi Zohar wrote:
> On Tue, 2026-05-26 at 15:51 +0200, 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.
> > To verify the template data hash value, convert the buffer policy data
> > to binary:
> > grep "ima_policy_loaded" \
> > 	/sys/kernel/security/integrity/ima/ascii_runtime_measurements | \
> > 	tail -1 | cut -d' ' -f 6 | xxd -r -p | sha256sum
> > 
> > Signed-off-by: Enrico Bravi <[email protected]>
> 
> Thanks, Enrico.  Just a few inline comments.

Thank you very much for your feedback.

> > ---
> >  security/integrity/ima/ima.h        |  1 +
> >  security/integrity/ima/ima_efi.c    |  2 ++
> >  security/integrity/ima/ima_fs.c     |  1 +
> >  security/integrity/ima/ima_policy.c | 55 +++++++++++++++++++++++++++--
> >  4 files changed, 57 insertions(+), 2 deletions(-)
> > 
> > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h
> > index 89ebe98ffc5e..a223d3f30d88 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..8e9f85ec9a86 100644
> > --- a/security/integrity/ima/ima_efi.c
> > +++ b/security/integrity/ima/ima_efi.c
> > @@ -60,6 +60,8 @@ static const char * const sb_arch_rules[] = {
> >  #endif
> >  #if IS_ENABLED(CONFIG_INTEGRITY_MACHINE_KEYRING) &&
> > IS_ENABLED(CONFIG_IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY)
> >  	"appraise func=POLICY_CHECK appraise_type=imasig",
> > +#else
> > +	"measure func=CRITICAL_DATA label=ima_policy",
> >  #endif
> 
>  None of the other arch "measure" policy rules are conditional.  Should the
> new
> "measure" rule be limited?

This condition aims to avoid measuring the policy loaded even if a signed policy
is required. In that case, it would not be possible to directly write the policy
in the securityfs file.

> >  	"measure func=MODULE_CHECK",
> >  	NULL
> > diff --git a/security/integrity/ima/ima_fs.c
> > b/security/integrity/ima/ima_fs.c
> > index 012a58959ff0..75cb308cf01f 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 bf2d7ba4c14a..e0b4dae922b6 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"
> > @@ -2022,7 +2023,6 @@ const char *const func_tokens[] = {
> >  	__ima_hooks(__ima_hook_stringify)
> >  };
> >  
> > -#ifdef	CONFIG_IMA_READ_POLICY
> 
> Removing the ifdef, here, does not affect viewing the IMA measurement lists,
> but
> allows copying and measuring the policy rules.  Please include a comment in
> the
> patch description.

Sure, will add in the next version.

> >  enum {
> >  	mask_exec = 0, mask_write, mask_read, mask_append
> >  };
> > @@ -2324,7 +2324,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)
> >  /*
> > @@ -2381,3 +2380,55 @@ bool ima_appraise_signature(enum kernel_read_file_id
> > id)
> >  	return found;
> >  }
> >  #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */
> > +
> 
> Please add kernel-doc here, something like:
> 
> /**
>  * ima_measure_loaded_policy - measure the active IMA policy ruleset
>  *
>  * Must be called with ima_write_mutex held, as it performs two
>  * separate RCU read passes over ima_rules and relies on the mutex
>  * to prevent concurrent policy updates between them.
>  */

Sure, thank you. If it is ok for you I can directly add what you suggested.

> > +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;
> > +	char rule[255];
> 
> The 255-byte buffer may be insufficient for custom policy rules that include
> additional fields such as LSM labels and other file metadata, unlike the
> simpler
> built-in and architecture-specific rules. Please increase the buffer size to
> accommodate the worst-case serialized rule length.

Yes, I wrongly took as reference the arch policy rules case. I don't know if the
worst-case can be precisely estimated. I could increase the buffer size and
check in any case if seq_has_overflowed(). Could it be an idea?

> > +
> > +	/* calculate IMA policy rules memory size */
> > +	file.buf = rule;
> > +	file.read_pos = 0;
> > +	file.size = 255;
> > +	file.count = 0;
> > +
> 
> Please add "lockdep_assert_held(&ima_write_mutex);"  here.

Yes, and this would actually fail because I'm not acquiring ima_write_mutex in
ima_release_policy().

> > +	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);
> > +		file_len += file.count;
> > +		file.count = 0;
> > +	}
> 
> Variables defined on the stack need to be initialized before being used.
> Please
> iniitalize file_len to zero.

Sure, will fix that.

Thank you,

Enrico

> > +	rcu_read_unlock();
> > +
> > +	/* copy IMA policy rules to a buffer for measuring */
> > +	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);
> > +}
> 
> Thanks,
> 
> Mimi