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