Re: [PATCH v6 0/2] firmware_loader: allow firmware_class.path to take multiple paths

[Jeff Layton]

On Thu, 2026-04-02 at 06:13 +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 01, 2026 at 09:43:26AM -0400, Jeff Layton wrote:
> > This is something Michal had asked for last year, and I just got around
> > to implementing. This version is just a minor cleanup for
> > reviewability's sake. I also renamed the search= option to search_path=.
> > 
> > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > ---
> 
> While it is still experimental, and might not be fully correct, you
> might want to look here:
> 	https://sashiko.dev/#/patchset/20260401-fw-path-v6-0-4ebe70441839%40kernel.org
> to see a few potential issues for this patchset (notibly at least the
> bash stuff)
> 

I'll plan to fix up the bash ones.

This race in the C review looks legit too:

------------------8<-------------------
-
> +	/* Try the customized path first */
> +	if (fw_path_para[0]) {
> +		int dirlen = strlen(fw_path_para);
>  
> -		fw_priv->size = 0;
> +		/* strip trailing newline */
> +		if (fw_path_para[dirlen - 1] == '\n')
> +			dirlen--;

Can a concurrent sysfs write cause an out-of-bounds read here?

If a sysfs write clears the string (e.g., by echoing an empty string to the
path parameter) immediately after the if (fw_path_para[0]) check,
strlen(fw_path_para) could return 0.

This would cause the code to evaluate fw_path_para[-1] == '\n', accessing
memory before the array bounds.
------------------8<-------------------

My reviews with Claude noticed the first race too, but I had concluded
that it was benign (you'd get a failed firmware load, but it shouldn't
crash).

I think that the simplest fix would be to just add a global rwsem
around the get/set routines. I'll plan to send a v7.

Thanks for the pointer!
-- 
Jeff Layton <jlayton@xxxxxxxxxx>