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
