Messages in this thread Patch in this message | | | Date | Tue, 15 Nov 2022 11:35:10 -0800 | From | Luis Chamberlain <> | Subject | Re: [syzbot] KASAN: use-after-free Read in kernfs_next_descendant_post (2) |
| |
On Mon, Nov 14, 2022 at 10:07:02AM -0800, Dmitry Torokhov wrote: > I do not see how moving the point where we acquire device refcount > around fixes anything.
The patch I posted does two things, moving the point where we acquire device refcount was just one so it was not clear that what I really wanted to be enforce a check for first, and that is that the driver *did* do the correct thing.
So while we can surely expect the driver to do proper device refcounting and waiting on device removal, buggy drivers do exist and we should strive to not allow UAF with them.
So something like this:
From 92c8f4465a205e744c70dcba320708f72900442e Mon Sep 17 00:00:00 2001 From: Luis Chamberlain <mcgrof@kernel.org> Date: Tue, 15 Nov 2022 10:02:13 -0800 Subject: [PATCH] firmware_loader: avoid UAF on buggy request_firmware_nowait() users
request_firmware_nowait() is documented as requiring the caller to ensure to maintain the the reference count of @device during the lifetime of the call to request_firmware_nowait() and the callback.
It would seem drivers exist which don't follow these rules though, and things like syzbot can trigger UAF if the device gets nuked as request_firmware_nowait() is being called. Instead of enabling use UAF, defend against such improperly written drivers and complain about it.
Make the documentaiton a bit clearer and give a hint as to how to easily accomplish device lifetime maintenance on the driver using a completion and a wait_for_completion().
Fixes: 0cfc1e1e7b534 ("firmware loader: fix device lifetime") Fixes: f8a4bd3456b98 ("firmware loader: embed device into firmware_priv structure") Cc: stable@vger.kernel.org # v2.6.36 Reported-by: syzbot+6bc35f3913193fe7f0d3@syzkaller.appspotmail.com Signed-off-by: Luis Chamberlain <mcgrof@kernel.org> --- drivers/base/firmware_loader/main.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c b/drivers/base/firmware_loader/main.c index 7c3590fd97c2..6ac92dfdd85e 100644 --- a/drivers/base/firmware_loader/main.c +++ b/drivers/base/firmware_loader/main.c @@ -1118,15 +1118,16 @@ static void request_firmware_work_func(struct work_struct *work) * @uevent: sends uevent to copy the firmware image if this flag * is non-zero else the firmware copy must be done manually. * @name: name of firmware file - * @device: device for which firmware is being loaded + * @device: device for which firmware is being loaded. The caller must hold + * the reference count of @device during the lifetime of this routine + * and the @cont callback. This typically can be done with a completion + * and wait_for_completion prior to device teardown. * @gfp: allocation flags * @context: will be passed over to @cont, and * @fw may be %NULL if firmware request fails. * @cont: function will be called asynchronously when the firmware * request is over. * - * Caller must hold the reference count of @device. - * * Asynchronous variant of request_firmware() for user contexts: * - sleep for as small periods as possible since it may * increase kernel boot time of built-in device drivers @@ -1171,7 +1172,12 @@ request_firmware_nowait( return -EFAULT; } - get_device(fw_work->device); + if (WARN_ON(!get_device(fw_work->device))) { + module_put(module); + kfree_const(fw_work->name); + kfree(fw_work); + return -ENODEV; + } INIT_WORK(&fw_work->work, request_firmware_work_func); schedule_work(&fw_work->work); return 0; -- 2.35.1
| |