Hi Mimi,
On Thu, 2026-06-11 at 10:30 -0400, Mimi Zohar wrote:
> 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.
Sure, will do.
> >
> > > > "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.
Perfect, thank you.
Enrico
> >
> > > > +
> > > > + /* 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
