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