Re: [PATCH] ima: prevent concurrent list operations in ima_lsm_update_rules

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

On Tue, 2025-05-27 at 20:51 +0800, Zhao Yipeng wrote:
> The current implementation of IMA policy list replacement via
> list_replace_rcu may trigger general protection faults under concurrent
> load policy operations. This occurs when a process replaces a node in
> ima_policy_rules list and sets old->prev = LIST_POISON2, while another
> parallel process still holds references to the old node. Subsequent list
> operations on the poisoned pointer result in kernel panic due to invalid
> memory access.
> 
> To resolve this, introduce a mutex lock (ima_rules_mutex) in
> ima_lsm_update_rules() to protect. ima_update_policy() also use the
> ima_policy_rules. Introduce a mutex lock in it.

A new IMA policy may replace the existing builtin policy rules with a custom
policy. In all other cases, the IMA policy rules may only be extended.  Writing
or extending the IMA policy rules requires taking the ima_write_mutex.

There's no need for a new mutex.

Mimi

> 
> Fixes: b16942455193 ("ima: use the lsm policy update notifier")
> Signed-off-by: Zhao Yipeng <zhaoyipeng5@huawei.com>
> ---
>  security/integrity/ima/ima_policy.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/security/integrity/ima/ima_policy.c
> b/security/integrity/ima/ima_policy.c
> index 128fab897930..d27e615e97d5 100644
> --- a/security/integrity/ima/ima_policy.c
> +++ b/security/integrity/ima/ima_policy.c
> @@ -471,6 +471,8 @@ static bool ima_rule_contains_lsm_cond(struct
> ima_rule_entry *entry)
>  	return false;
>  }
>  
> +static DEFINE_MUTEX(ima_rules_mutex);
> +
>  /*
>   * The LSM policy can be reloaded, leaving the IMA LSM based rules referring
>   * to the old, stale LSM policy.  Update the IMA LSM based rules to reflect
> @@ -481,16 +483,19 @@ static void ima_lsm_update_rules(void)
>  	struct ima_rule_entry *entry, *e;
>  	int result;
>  
> +	mutex_lock(&ima_rules_mutex);
>  	list_for_each_entry_safe(entry, e, &ima_policy_rules, list) {
>  		if (!ima_rule_contains_lsm_cond(entry))
>  			continue;
>  
>  		result = ima_lsm_update_rule(entry);
>  		if (result) {
> +			mutex_unlock(&ima_rules_mutex);
>  			pr_err("lsm rule update error %d\n", result);
>  			return;
>  		}
>  	}
> +	mutex_unlock(&ima_rules_mutex);
>  }
>  
>  int ima_lsm_policy_change(struct notifier_block *nb, unsigned long event,
> @@ -1038,9 +1043,12 @@ int ima_check_policy(void)
>   */
>  void ima_update_policy(void)
>  {
> -	struct list_head *policy = &ima_policy_rules;
> +	struct list_head *policy;
>  
> +	mutex_lock(&ima_rules_mutex);
> +	policy = &ima_policy_rules;
>  	list_splice_tail_init_rcu(&ima_temp_rules, policy, synchronize_rcu);
> +	mutex_unlock(&ima_rules_mutex);
>  
>  	if (ima_rules != (struct list_head __rcu *)policy) {
>  		ima_policy_flag = 0;