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

Jeff Layton
2026-04-02T16:51:45Z

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@kernel.org>
> > ---
>
> 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@kernel.org>