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

[Mimi Zohar <zohar@linux.ibm.com>]

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.

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.

Please include directions for verifying the critical-data (e.g. using xxd).

> 
> Signed-off-by: Enrico Bravi <enrico.bravi@polito.it>
> ---
>  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?

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

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