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: <aPdUAOirD2yodeTy@pluto>
Date: Tue, 21 Oct 2025 10:36:00 +0100
From: Cristian Marussi <cristian.marussi@....com>
To: Jonathan Cameron <jonathan.cameron@...wei.com>
Cc: Cristian Marussi <cristian.marussi@....com>,
	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 Fri, Oct 17, 2025 at 04:07:02PM +0100, Jonathan Cameron wrote:
> 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!)

Hi Jonathan,

thanks for having a look at this series.

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

Thanks for the heads-up I will dig better into cleanup.h which obviously
I did not enough...my bad.

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

You are right...also the Dox comment is obsoleted..I will review this
patch as a whole, since even if probably right in its essence is badly
implemented because I rushed it in in this series to fix a probblem that
popped up on KASAN.

> 
> > +			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, protoo);

Yes this seems a better rework...

Thanks,
Cristian