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