firmware_loader: fix use-after-free in fw_load_sysfs_fallback()

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.

---
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(+)

index 3ef0b312ae71..997ee8d360f7 100644
@@ -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

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

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

Commit 9f10374bb024 ("can: remove private CAN skb headroom infrastructure") removed the skb_reset_mac_header()/skb_reset_network_header()/ skb_reset_transport_header() calls from init_can_skb(). As a result, RX skbs from alloc_can_skb() and friends again carry mac_header = 0xFFFF. When such an skb reaches packet_rcv_spkt() (SOCK_PACKET), the push length calculation overflows and triggers skb_under_panic -> kernel BUG -> full machine panic.

The same issue was originally reported in 2014 on linux-can and fixed by commit 969439016d2c ("can: add missing initialisations in CAN related skbuffs"). packet_rcv_spkt() itself has never been hardened: only packet_rcv() and tpacket_rcv() gained dev_has_header() checks in commit d549699048b4 ("net/packet: fix packet receive on L3 devices without visible hard header").

---
v2:
- Remove in-code comment per Oliver's feedback.
- Pick up Reviewed-by and Acked-by tags from Oliver Hartkopp.

 drivers/net/can/dev/skb.c | 4 ++++
 1 file changed, 4 insertions(+)

index 95fcdc1026f8..14edec5afb57 100644
@@ -210,6 +210,10 @@ static void init_can_skb(struct sk_buff *skb)
 {
 	skb->pkt_type = PACKET_BROADCAST;
 	skb->ip_summed = CHECKSUM_UNNECESSARY;
+
+	skb_reset_mac_header(skb);
+	skb_reset_network_header(skb);
+	skb_reset_transport_header(skb);
 }
 
 struct sk_buff *alloc_can_skb(struct net_device *dev, struct can_frame **cf)

2.53.0

Hello zjamg,

I reproduced the panic independently and verified that your patch alone fixes it, so:

How I tested, in case it is useful for the maintainers. The driver RX path is required: vcan does not reproduce it, because can_send() resets the headers itself on the way out. I used slcan over a pty, opened a SOCK_PACKET socket on the resulting can0, and pushed one frame in through the line discipline. Two kernels, same config, same initramfs, only your patch differing.

Without the patch, v7.3.0-rc5 under QEMU:

skbuff: skb_under_panic: text:ffffffffa0b28261 len:-65455 put:-65471 head:... data:... tail:0x50 end:0x180 dev:can0 kernel BUG at net/core/skbuff.c:214!

  RIP: 0010:skb_panic+0x50/0x60
  Call Trace:
   <TASK>
   skb_push+0x4d/0x60
   packet_rcv_spkt+0xe1/0x170
  Kernel panic - not syncing: Fatal exception in interrupt

(the len/put values match the ones in your report)

With your v2 applied verbatim, the identical run prints

RESULT: survived, no skb_under_panic

and the guest powers off normally.

One note that may be worth adding to the commit message if you respin: the 2015 fix you reference, 969439016d2c, is the second time this class of bug was closed on the producer side. packet_rcv_spkt() has still never been hardened, which is why the same failure mode came back a third time when 9f10374bb024 dropped the calls again. I sent a separate patch for that receiver-side guard so the next regression of this kind degrades to a dropped frame instead of a panic - no overlap with this one, and yours is the one that fixes the actual regression.

Thanks for picking this up,
Quchaosheng

isotp_rcv() separates Classic CAN from CAN FD by skb->len alone:

	if (skb->len != so->ll.mtu)
		return;

	cf = (struct canfd_frame *)skb->data;

A CAN XL frame with cxl->len 4 is CAN_MTU bytes, so it passes, and is then read as a canfd_frame whose len comes out of canxl_frame.flags: at least 0x80.

Of the paths that follow, only the flow control one uses that length without bounding it first, so check_pad() walks to 255 over a 16-byte frame and the caller reports EBADMSG on an unrelated socket.

bcm_rx_handler(), j1939_can_recv(), can_can_gw_rcv() and raw_rcv() check the frame type here, and can_dropped_invalid_skb() switches on skb->protocol on the transmit side. isotp_rcv() is the gap.

---
v2: shorten the comment above the new check to say what it does; the
reasoning stays in the description (Oliver Hartkopp). Add Oliver's Reviewed-by and Acked-by. No code change.

v1: https://lore.kernel.org/linux-can/[email protected]/

Reproduced on v7.2.4 over vcan, one isotp socket per case bound rx 0x123 with RX_PADDING|CHK_PAD_DATA and rxpad_content 0xAA, a first frame in flight, and one frame injected from a CAN_RAW socket.

	case                            stock     patched
	A CAN XL, cxl->len 4, flags ff  EBADMSG   none
	B Classic FC, padded 0xAA       none      none
	C Classic FC, padded 0x00       EBADMSG   EBADMSG
	D as A, with CHK_PAD_LEN on     EBADMSG   none

C bounds the impact: a malformed Classic FC frame from any sender on the bus gives the same EBADMSG, so nothing becomes reachable that was not already. D differs only in which branch of check_pad() returns.

No memory safety issue. KASAN was on for all eight runs and reported nothing.

---
 net/can/isotp.c | 8 ++++++++
 1 file changed, 8 insertions(+)

@@ -754,8 +754,16 @@ static void isotp_rcv(struct sk_buff *skb, void *data)
 	 */
 	if (skb->len != so->ll.mtu)
 		return;
 
+	/* check for correct CAN CC/FD frame content */
+	if (so->ll.mtu == CAN_MTU) {
+		if (!can_is_can_skb(skb))
+			return;
+	} else if (!can_is_canfd_skb(skb)) {
+		return;
+	}
+
 	cf = (struct canfd_frame *)skb->data;
 
 	/* if enabled: check reception of my configured extended address */
 	if (ae && cf->data[0] != so->opt.rx_ext_address)

Thanks for the fast update!

Awaiting upstream.

Best regards,
Oliver

Hello Kaixuan,

I hit the same collision from a different direction and independently arrived at the same fix, so:

Two things I checked that may be worth having on the record, since they are the questions this patch is likely to attract.

First, that isotp really is the only gap, so it does not need to grow into a series. I went through the other places that consume a CAN skb without looking at its type. bcm_rx_handler() and can_can_gw_rcv() do gate on can_is_can_skb() / can_is_canfd_skb(), and those two helpers carry a second condition on the frame length field (include/linux/can/skb.h:93 and :101). On a CAN XL frame that field is cxl->flags, and CANXL_XLF alone is 0x80, which is already past CAN_MAX_DLEN and CANFD_MAX_DLEN, so an XL frame is rejected there before its length is ever used. So the collision is specific to isotp.

Second, nothing on the transmit side can produce it. isotp always builds its frames at so->ll.mtu and validates ll.mtu against CAN_MTU / CANFD_MTU in the setsockopt path, so the frame that passes the old length-only test can only come off the wire.

Your A/B table is what convinced me, by the way - case C in particular is the right control, since it shows the EBADMSG already existed for a malformed Classic FC from any sender. Nothing becomes reachable that was not reachable before.

Thanks for fixing this,
Quchaosheng

Re-checked this against a 7.3-rc7 tree before it went in; recording what was measured.

The field offsets in the message are right:

  sizeof(struct can_frame)    16    CAN_MTU
  sizeof(struct canfd_frame)  72    CANFD_MTU

canxl_frame: data at 12, len at 6, flags at 4

canxl->len == 4 makes a 16-byte skb, which is CAN_MTU and passes "skb->len != so->ll.mtu" on a Classic channel. Cast to struct canfd_frame, the byte at offset 4 is canxl->flags, and CANXL_XLF is 0x80 and mandatory, so cf->len is 0xff in the run below. The flow-control path uses that length without bounding it, which is what check_pad() walks to 255 over.

Measured on v7.3-rc7 under QEMU/KVM on vcan with mtu 2060, two kernels from one tree differing only in this hunk:

A a CAN XL frame with len 4 injected into a Classic socket with
RX_PADDING | CHK_PAD_LEN | CHK_PAD_DATA:
without the patch recv() -> EBADMSG
with the patch no EBADMSG
B legitimate traffic and the padding check:
CC ok-unpadded / ok-padded / wrong-padding intact, intact, EBADMSG
FD ok-unpadded / ok-padded / wrong-padding intact, intact, EBADMSG
identical on both kernels.

B matters because the guard sits in front of the code that reports bad padding. The wrong-padding cases still report EBADMSG on the patched kernel, and the CAN FD channel is unaffected.