linux-kernel - Re: [PATCH 02/10] firmware: arm_scmi: Reduce the scope of protocols mutex

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20251017160702.00002af4@huawei.com>
Date: Fri, 17 Oct 2025 16:07:02 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Cristian Marussi <cristian.marussi@....com>
CC: <linux-kernel@...r.kernel.org>, <linux-arm-kernel@...ts.infradead.org>,
	<arm-scmi@...r.kernel.org>, <sudeep.holla@....com>,
	<james.quinlan@...adcom.com>, <f.fainelli@...il.com>,
	<vincent.guittot@...aro.org>, <etienne.carriere@...com>,
	<peng.fan@....nxp.com>, <michal.simek@....com>, <quic_sibis@...cinc.com>,
	<dan.carpenter@...aro.org>, <d-gole@...com>, <souvik.chakravarty@....com>
Subject: Re: [PATCH 02/10] firmware: arm_scmi: Reduce the scope of protocols
 mutex

On Thu, 25 Sep 2025 21:35:46 +0100
Cristian Marussi <cristian.marussi@....com> wrote:

> Currently the mutex dedicated to the protection of the list of registered
> protocols is held during all the protocol initialization phase.
> 
> Such a wide locking region is not needed and causes problem when trying to
> initialize notifications from within a protocol initialization routine.
> 
> Reduce the scope of the protocol mutex.
> 
> Signed-off-by: Cristian Marussi <cristian.marussi@....com>

Hi Cristian.  A few things inline and this runs into one of the things
that is dangerous to do with guard() or the other cleanup.h magic
(and documented there!)

> ---
>  drivers/firmware/arm_scmi/driver.c | 53 +++++++++++++++---------------
>  1 file changed, 26 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c
> index bd56a877fdfc..71ee25b78624 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -17,6 +17,7 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/bitmap.h>
> +#include <linux/cleanup.h>
>  #include <linux/debugfs.h>
>  #include <linux/device.h>
>  #include <linux/export.h>
> @@ -2179,10 +2180,13 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  	if (ret)
>  		goto clean;
>  
> -	ret = idr_alloc(&info->protocols, pi, proto->id, proto->id + 1,
> -			GFP_KERNEL);
> -	if (ret != proto->id)
> -		goto clean;
> +	/* Finally register the initialized protocol */
> +	scoped_guard(mutex, &info->protocols_mtx) {

See the guidance in cleanup.h on mixing goto and anything defined in that file.

In some compilers, if you hit the goto above and hence jump over this
the cleanup variable will still be instantiated, but not initialized leading to
a potential attempt to unlock random memory.

Either this needs more substantial rework, or just handling the mutex with
out using guards.


> +		ret = idr_alloc(&info->protocols, pi, proto->id, proto->id + 1,
> +				GFP_KERNEL);
> +		if (ret != proto->id)
> +			goto clean;
> +	}
>  
>  	/*
>  	 * Warn but ignore events registration errors since we do not want
> @@ -2243,25 +2247,22 @@ scmi_alloc_init_protocol_instance(struct scmi_info *info,
>  static struct scmi_protocol_instance * __must_check
>  scmi_get_protocol_instance(const struct scmi_handle *handle, u8 protocol_id)
>  {
> -	struct scmi_protocol_instance *pi;
> +	struct scmi_protocol_instance *pi = ERR_PTR(-EPROBE_DEFER);
>  	struct scmi_info *info = handle_to_scmi_info(handle);
> +	const struct scmi_protocol *proto;
>  
> -	mutex_lock(&info->protocols_mtx);
> -	pi = idr_find(&info->protocols, protocol_id);
> -
> -	if (pi) {
> -		refcount_inc(&pi->users);
> -	} else {
> -		const struct scmi_protocol *proto;
> -
> -		/* Fails if protocol not registered on bus */
> -		proto = scmi_protocol_get(protocol_id, &info->version);
> -		if (proto)
> -			pi = scmi_alloc_init_protocol_instance(info, proto);
> -		else
> -			pi = ERR_PTR(-EPROBE_DEFER);
> +	scoped_guard(mutex, &info->protocols_mtx) {
> +		pi = idr_find(&info->protocols, protocol_id);
> +		if (pi) {

if !pi we carry on with it NULL, which is a behavior change from
before where it would be ERR_PTR(-EPROBE_DEFER);

That might not matter, but it's not 'obviously' a safe change.

> +			refcount_inc(&pi->users);
> +			return pi;
> +		}
>  	}
> -	mutex_unlock(&info->protocols_mtx);
> +
> +	/* Fails if protocol not registered on bus */
> +	proto = scmi_protocol_get(protocol_id, &info->version);
> +	if (proto)
Trivial but I'd flip the logic to
	if (!proto)
		return ERR_PTR(-EPROBE_DEFER);
assuming a NULL return as mentioned above isn't the intent.
Then
	return scmi_alloc_init_protocol_instance(info, proto);

> +		pi = scmi_alloc_init_protocol_instance(info, proto);
>  
>  	return pi;
>  }