[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

[PATCH v2] can: restore skb header initialisations in init_can_skb()

[zjamg]

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

Fixes: 9f10374bb024 ("can: remove private CAN skb headroom infrastructure")
Cc: stable@vger.kernel.org
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Signed-off-by: zjamg <ndaugoing@gmail.com>
---
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(+)

diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c
index 95fcdc1026f8..14edec5afb57 100644
--- a/drivers/net/can/dev/skb.c
+++ b/drivers/net/can/dev/skb.c
@@ -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

Re: [PATCH v2] can: restore skb header initialisations in init_can_skb()

[Quchaosheng]

Hello zjamg,

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

Tested-by: Quchaosheng <quchaosheng000406@163.com>

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

[PATCH v2] can: isotp: check the frame type, not just the length

[Kaixuan Li]

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.

Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
---
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/20260919122852.1868961-1-kaixuanli0131@gmail.com/

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

--- a/net/can/isotp.c
+++ b/net/can/isotp.c
@@ -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)

Re: [PATCH v2] can: isotp: check the frame type, not just the length

[Oliver Hartkopp]

On 20.09.26 05:56, Kaixuan Li wrote:
> 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.
> 
> Fixes: fb08cba12b52 ("can: canxl: update CAN infrastructure for CAN XL frames")
> Signed-off-by: Kaixuan Li <kaixuanli0131@gmail.com>
> Reviewed-by: Oliver Hartkopp <socketcan@hartkopp.net>
> Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
> ---
> 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.
> 

Thanks for the fast update!

Awaiting upstream.

Best regards,
Oliver

> v1: https://lore.kernel.org/linux-can/20260919122852.1868961-1-kaixuanli0131@gmail.com/
> 
> 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(+)
> 
> --- a/net/can/isotp.c
> +++ b/net/can/isotp.c
> @@ -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)

Re: [PATCH v2] can: isotp: check the frame type, not just the length

[Quchaosheng]

Hello Kaixuan,

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

Reviewed-by: Quchaosheng <quchaosheng000406@163.com>

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