On Fri, 2025-12-19 at 14:13 -0500, Mimi Zohar wrote: > On Tue, 2025-12-16 at 17:56 +0100, 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.
Hi Mimi, sorry for the delay in answering you. > The IMA policy could be really large. Do we really want to include all the > policy rules in the template data? The other option would be to include a > hash of the policy rules, in lieu of the policy rules themselves. I thought to include all the policy rules because the policy can be defined using the same rules in a different order. For large policy this would mean that a verifier should have several hash values for the same policy. > Please include directions for verifying the critical-data (e.g. using xxd). Yes, sure. > > > > Signed-off-by: Enrico Bravi <[email protected]> > > --- > > security/integrity/ima/ima.h | 1 + > > security/integrity/ima/ima_efi.c | 1 + > > security/integrity/ima/ima_fs.c | 1 + > > security/integrity/ima/ima_policy.c | 63 ++++++++++++++++++++++++++++- > > 4 files changed, 64 insertions(+), 2 deletions(-) > > > > diff --git a/security/integrity/ima/ima.h b/security/integrity/ima/ima.h > > index e3d71d8d56e3..ca7b96663623 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..199c42d0f8b3 100644 > > --- a/security/integrity/ima/ima_efi.c > > +++ b/security/integrity/ima/ima_efi.c > > @@ -62,6 +62,7 @@ static const char * const sb_arch_rules[] = { > > "appraise func=POLICY_CHECK appraise_type=imasig", > > #endif > > "measure func=MODULE_CHECK", > > + "measure func=CRITICAL_DATA label=ima_policy", > > With this rule, the policy will always be measured, even when loading a signed > policy file. It that necessary? No you're right. I can include it only in the case the policy is not apprised. > > NULL > > }; > > > > diff --git a/security/integrity/ima/ima_fs.c > > b/security/integrity/ima/ima_fs.c > > index 87045b09f120..89946d803d44 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 128fab897930..956063748008 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" > > @@ -1983,7 +1984,6 @@ const char *const func_tokens[] = { > > __ima_hooks(__ima_hook_stringify) > > }; > > > > -#ifdef CONFIG_IMA_READ_POLICY > > enum { > > mask_exec = 0, mask_write, mask_read, mask_append > > }; > > @@ -2277,7 +2277,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) > > /* > > @@ -2334,3 +2333,63 @@ bool ima_appraise_signature(enum kernel_read_file_id > > id) > > return found; > > } > > #endif /* CONFIG_IMA_APPRAISE && CONFIG_INTEGRITY_TRUSTED_KEYRING */ > > + > > +static size_t ima_policy_text_len(void) > > +{ > > + struct list_head *ima_rules_tmp; > > + struct ima_rule_entry *entry; > > + struct seq_file file; > > + size_t size = 0; > > + char rule[255]; > > + > > + file.buf = rule; > > + file.read_pos = 0; > > + file.size = 255; > > + file.count = 0; > > + > > + rcu_read_lock(); > > + ima_rules_tmp = rcu_dereference(ima_rules); > > + list_for_each_entry_rcu(entry, ima_rules_tmp, list) { > > + ima_policy_show(&file, entry); > > + size += file.count; > > + file.count = 0; > > + } > > + rcu_read_unlock(); > > + > > + return size; > > +} > > + > > +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 = ima_policy_text_len(); > > Normally a function is defined to prevent code duplication or for readability. > In this case, it doesn't achieve either. Ok, yes you're right. I'll move the ima_policy_text_len() code in ima_measure_loaded_policy(). Thank you very much for your feedback. Enrico > Add a comment here, something like: > /* calculate IMA policy rules memory size */ > > > + > > + file.buf = vmalloc(file_len); > > + if (!file.buf) { > > + integrity_audit_msg(AUDIT_INTEGRITY_PCR, NULL, event_name, > > + op, audit_cause, result, 1); > > + return; > > + } > > > > > > And another comment below, something like: > /* copy IMA policy rules to a buffer */ > > > + > > + 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
