linux-kernel - Re: [PATCH] ima: prevent concurrent list operations in ima_lsm_update

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <f04a8c07d7da1df2cd5b74f04d253611bab9f1ad.camel@linux.ibm.com>
Date: Tue, 27 May 2025 15:02:41 -0400
From: Mimi Zohar <zohar@...ux.ibm.com>
To: Zhao Yipeng <zhaoyipeng5@...wei.com>, roberto.sassu@...wei.com,
        dmitry.kasatkin@...il.com, eric.snowberg@...cle.com,
        paul@...l-moore.com, jmorris@...ei.org, serge@...lyn.com,
        janne.karhunen@...il.com
Cc: morgan@...nel.org, lujialin4@...wei.com, linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ima: prevent concurrent list operations in
 ima_lsm_update_rules

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@...wei.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;