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

[Mimi Zohar <zohar@linux.ibm.com>]

On Thu, 2026-06-11 at 12:51 +0000, Enrico  Bravi wrote:
> 
> > > 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.

Good point. Since it is different than the other rules, could you add a comment
here or in the patch description.

> 
> > >  	"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
> > > 
> > > @@ -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.

This was suggested by Claude, so it should be acceptable to use.

> 
> > > +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?

Sounds good.

> 
> > > +
> > > +	/* 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().

Thanks.

Mimi
> >