[RFC PATCH] firmware_loader: fix use-after-free in fw_load_sysfs_fallback()

[RFC PATCH] firmware_loader: fix use-after-free in fw_load_sysfs_fallback()

[Ian Bridges]

When a firmware request falls back to the sysfs interface,
fw_load_sysfs_fallback() registers a temporary firmware device as a child
of the requesting device and adds it with device_add(). For the
asynchronous request_firmware_nowait() path this runs from a workqueue.

request_firmware_nowait() takes a reference on the requesting device with
get_device(). That keeps its struct device allocated, but not its sysfs
directory, which device_del() tears down independently. So a device whose
driver requested firmware from its probe function can be removed, for
example by a USB disconnect, while the fallback work is still running.

When that happens, device_del() frees the requesting device's kernfs nodes.
Concurrently, device_add() reads those nodes and then takes a reference on
each with kernfs_get(). If a node is freed between the read and the
kernfs_get(), kernfs_get() runs on freed memory, a use-after-free.

Fix this by serializing the device_add() against the requesting device's
removal. device_del() sets the device's dead flag under its device lock
before it removes the directory, so take the requesting device's lock
across device_add() and return -ENODEV if the flag is already set.

Fixes: e55c8790d40f ("Driver core: convert firmware code to use struct device")
Reported-by: syzbot+3942dc5563ea8b96bbbe@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=3942dc5563ea8b96bbbe
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
This patch contains a proposed fix for a crash reported by syzbot in
__kernfs_new_node().

The file names and offsets in this description are from commit
c425609d6ac4012c8bbf01ec2e10e801b1923a7b of
git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git

I also have a test harness that triggers the crash from an unprivileged
USB device, an emulated ueagle-atm pre-firmware device driven through
USB Raw Gadget and dummy_hcd. The test harness uses memory pressure to
artificially extend the race window. The harness was written with the
help of a coding agent (Claude Code).

I've marked this patch as an RFC because I'm relatively new to working
with the Linux kernel and this is my first attempt at work in this
subsystem. Any feedback is appreciated.

The Bug

When a driver requests firmware that the kernel cannot load directly, the
loader can fall back to a sysfs interface for userspace to supply the
image. It registers a temporary firmware device, f_dev, as a child of the
requesting device (f_dev->parent). On the asynchronous
request_firmware_nowait() path, it adds that device with device_add()
from a workqueue.

request_firmware_nowait() takes a reference on the requesting device with
get_device() and holds it for the duration of the work. A device's sysfs
directory is a kernfs node, held in its kobject's sd field. device_add()
creates it and device_del() removes it, independently of the reference
count. So get_device() keeps the struct device allocated but not that
directory, and the requesting device can be removed while the work runs.
For example, a USB device whose driver requested firmware from its probe
function can be disconnected.

The work and the requester's removal run on different threads. They
interleave to trigger the bug as follows:

1. A driver calls request_firmware_nowait() from its probe function. The
   loader takes get_device() on the device and schedules the work on the
   events workqueue.

2. Probe returns. The device is now free to be removed at any time, for
   example a USB disconnect. The reference keeps only the struct device
   allocated, not its sysfs directory.

3. The work runs, fails the direct load, and enters the sysfs fallback in
   fw_load_sysfs_fallback() (fallback.c:75). f_dev->parent already points
   at the requesting device.

4. fw_load_sysfs_fallback() calls device_add(f_dev). Building f_dev's
   directory reaches sysfs_create_dir_ns(), which reads the requesting
   device's directory node from kobj->parent->sd.

5. Concurrently the requesting device is removed. device_del() runs
   kobject_del() to remove the directory. sysfs_remove_dir() first clears
   the requesting device's kobj->sd, the pointer step 4 just read, then
   kernfs_remove() frees the node through call_rcu() (fs/kernfs/dir.c:618).

6. device_add() takes a reference on that node, now freed, with kernfs_get()
   in __kernfs_new_node() (fs/kernfs/dir.c:704). kernfs_get() reads the
   node's count (kn->count, fs/kernfs/dir.c:560) on freed memory.

The read in step 4 had to happen before step 5 cleared the pointer. A read
afterward gets NULL and returns -ENOENT, so the window is between the read
in step 4 and the kernfs_get() in step 6. device_add() reads the parent
tree at more than one point, so the fault is not tied to a single caller.

The Proposed Fix

device_del() takes device_lock(dev) and calls kill_device()
(drivers/base/core.c:3884) to set dev->p->dead, then unlocks and removes the
directory later, outside that lock. So fw_load_sysfs_fallback() takes the
requesting device's lock across device_add() and checks that flag. If it is
set, the requester is already going away and device_add() is skipped,
returning -ENODEV. Otherwise the lock is held across device_add(), which
blocks the requester's device_del() at the same device_lock(), before it can
mark the device dead or free the directory, and keeps the requesting
device's sysfs tree alive for the duration.

The fix was verified with the test harness mentioned above.

As a side note, the comment above cleanup_glue_dir() in drivers/base/core.c
documents a similar race.

The Fixes tag is e55c8790d40f. The race is two paths touching the requesting
device's kobj->sd with no lock between them. device_del() has always cleared
that field and freed the node during teardown. This commit added the second
path. By setting f_dev->parent to the requesting device and calling
device_register(), it made device_add() read the same kobj->sd and take a
reference on the node with kernfs_get(). The earlier class_device placed the
fallback under the firmware class and never touched the requester's node, so
device_del() was the only accessor and there was nothing to race. The
asynchronous request_firmware_nowait() path and the get_device() lifetime
were already in place, so this commit, the one that added a second accessor
with no lock against the first, is where the race was introduced.

 drivers/base/firmware_loader/fallback.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c b/drivers/base/firmware_loader/fallback.c
index 3ef0b312ae71..997ee8d360f7 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -8,6 +8,7 @@
 #include <linux/sysctl.h>
 #include <linux/module.h>
 
+#include "../base.h"
 #include "fallback.h"
 #include "firmware.h"
 
@@ -75,6 +76,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
 {
 	int retval = 0;
 	struct device *f_dev = &fw_sysfs->dev;
+	struct device *parent = f_dev->parent;
 	struct fw_priv *fw_priv = fw_sysfs->fw_priv;
 
 	/* fall back on userspace loading */
@@ -83,7 +85,23 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs, long timeout)
 
 	dev_set_uevent_suppress(f_dev, true);
 
+	/*
+	 * The fallback device is added as a child of the requesting device,
+	 * which can be removed concurrently. Hold the requester's lock across
+	 * device_add() to serialize against its removal, and skip the add if
+	 * the requester is already dead.
+	 */
+	if (parent) {
+		device_lock(parent);
+		if (parent->p->dead) {
+			device_unlock(parent);
+			retval = -ENODEV;
+			goto err_put_dev;
+		}
+	}
 	retval = device_add(f_dev);
+	if (parent)
+		device_unlock(parent);
 	if (retval) {
 		dev_err(f_dev, "%s: device_register failed\n", __func__);
 		goto err_put_dev;
-- 
2.47.3

Re: [RFC PATCH] firmware_loader: fix use-after-free in fw_load_sysfs_fallback()

[Quchaosheng]

Hello Ian,

I went through the two syzbot reports this targets (kernfs_get and
__kernfs_new_node) and I agree with your analysis.  device_add() is not
serialized against the removal of the requesting device, and
get_device() in request_firmware_nowait() keeps only the struct device
alive, not the kernfs nodes behind dev->kobj.sd.  The window is real.

Unfortunately I think the patch as posted cannot go in as-is:
device_lock(parent) around device_add() deadlocks for any driver that
requests firmware from its probe function, which is the common case
rather than an edge case.

Why: ->probe() already runs with the device mutex held.

  __device_attach()
      device_lock(dev);                     <- device mutex taken
      bus_for_each_drv(... __device_attach_driver)
          driver_probe_device() -> really_probe()
              drv->probe(dev)               <- driver probe

A driver that calls request_firmware() from there reaches

  _request_firmware()
      firmware_fallback_sysfs()
          fw_load_from_user_helper()
              fw_load_sysfs_fallback()
                  device_lock(parent)       <- parent == that same dev

and takes the mutex it is already holding.  Linux mutexes are not
recursive (see the semantics list in include/linux/mutex_types.h), so it
self-deadlocks.  Both the synchronous and the nowait path end up in
fw_load_sysfs_fallback(), so both are affected.

I measured this rather than reasoning about it.  Two kernels, same
config (CONFIG_PROVE_LOCKING + CONFIG_DEBUG_MUTEXES), same initramfs.  I
wrote a scratch platform driver whose ->probe() has the same shape as
softing_pdev_probe() -> softing_card_boot() -> softing_load_fw() ->
request_firmware() (drivers/net/can/softing/softing_fw.c:153), and made
it print mutex_is_locked(&dev->mutex) before requesting firmware.

Without the patch, the probe reports the mutex is held and then completes:

  fwlockdep-test: probe: mutex_is_locked(&dev->mutex) = 1
  fwlockdep-test: Falling back to sysfs fallback for: fwlockdep/does-not-exist.bin
  fwlockdep-test: probe: request_firmware returned -110      <- returned

With your RFC applied verbatim, the same probe reaches the fallback and
never comes back:

  fwlockdep-test: probe: mutex_is_locked(&dev->mutex) = 1
  fwlockdep-test: Falling back to sysfs fallback for: fwlockdep/does-not-exist.bin
  (no further output; guest had to be killed after 320s)

The host run timed out (qemu exit 124) and the guest never reached the
"request_firmware returned" line, so the 60s firmware timeout never even
expires - the task is stuck on the mutex, not waiting for userspace.

Some directions that might work instead:

a) Keep a reference on the parent's kernfs node / directory rather than
   on the struct device, so the node cannot be freed while device_add()
   uses it.  The fallback device currently holds the parent device, and
   device_del() tears the kobject hierarchy down independently of that
   reference, which is the asymmetry to close.

b) Have device_del()-side teardown and the fallback device_add()
   synchronize on something that is not the device mutex, since the
   mutex is already held by every probe-context caller.

c) Detect the teardown after device_add() and unwind, which would stop
   the crash without mutual exclusion, though it leaves the kernfs node
   lifetime question open.

I have not written a patch for this.  I would rather help get yours into
shape than file a competing one.  Raising the deadlock now seems better
than having a maintainer spend a review cycle on the RFC and hit it.

If you still have the dummy_hcd + Raw Gadget harness you mentioned, that
would be very useful for validating whichever direction we pick against
the actual crash.  Happy to dig into one of these with you.

Thanks,
Quchaosheng

Re: [RFC PATCH] firmware_loader: fix use-after-free in fw_load_sysfs_fallback()

[Ian Bridges]

Hi Quchaosheng,

Thanks for the review, and for measuring it rather than reasoning about
it. The deadlock is real and this approach is a dead end, so I am
withdrawing the patch.

I reproduced it on a clean v7.0.1 with the RFC applied and
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y, using a platform driver whose
probe() issues a synchronous request_firmware() for a missing file. The
probe runs with device_lock(dev) already held, request_firmware() falls
through to fw_load_sysfs_fallback(), and that takes device_lock(parent)
on the same device. The task ends up in uninterruptible sleep in
__mutex_lock, reached from fw_load_sysfs_fallback() inside
request_firmware(), called from the driver's probe(), which really_probe()
had entered after __device_attach() already took device_lock(dev). So it
is the same mutex taken twice by the same task. request_firmware() never
returns, and the firmware timeout never matters because the task is stuck
on the mutex rather than waiting for userspace, exactly as you said.

One thing worth mentioning for anyone reproducing this is that lockdep
will not flag it. device_initialize() marks dev->mutex with
lockdep_set_novalidate_class() in drivers/base/core.c, so PROVE_LOCKING
cannot report the recursive acquisition. The silent hang is the only
signature.

The two paths do differ. The reported syzbot crash is the asynchronous
request_firmware_nowait() path, where fw_load_sysfs_fallback() runs from
a workqueue with no device_lock held. The lock I added sits in code that
both callers share, so it serializes the async path but deadlocks the
synchronous one.

I do not think there is a one line fix here, which is the main reason I
am not sending a v2. The underlying problem is that device_add() touches
the requesting device's kernfs nodes in more than one place with nothing
holding them alive, while device_del() frees them through a recursive
teardown. sysfs_create_dir_ns() reads kobj->parent->sd and takes a
reference on it before kernfs_add_one(). Then create_dir() takes another
reference on kobj->sd after the add, by which point __kernfs_remove() on
the parent can already have reaped the child that was just added. A
kernfs_get_unless_zero() try-get in sysfs_create_dir_ns(), which was your
direction (a), closes the first site but the fault just moves to the
second. Covering every site means touching generic kobject and kernfs
code, and serializing device_add() against the teardown cannot use the
device mutex, as this deadlock shows, without blocking disconnect for the
fallback timeout. As far as I can tell, this is really a driver core and
kernfs design question, so I would rather not throw a third mechanism at it
without a steer from the maintainers.

Thanks,
Ian