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

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.

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

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

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

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

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

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