uhid has to run hid_add_device() from workqueue context while allowing
parallel use of the userspace API (which is protected with ->devlock).
But hid_add_device() can fail. Currently, that is handled by immediately
destroying the associated HID device, without using ->devlock - but if
there are concurrent requests from userspace, that's wrong and leads to
NULL dereferences and/or memory corruption (via use-after-free).
Fix it by leaving the HID device as-is in the worker. We can clean it up
later, either in the UHID_DESTROY command handler or in the ->release()
handler.
Cc: [email protected]
Fixes: 67f8ecc550b5 ("HID: uhid: fix timeout when probe races with IO")
Signed-off-by: Jann Horn <[email protected]>
---
Notes:
This crasher triggers an ASAN UAF warning:
int main(void) {
int dev = open("/dev/uhid", O_RDWR);
if (dev == -1) err(1, "open");
while (1) {
struct uhid_event ev_create = {
.type = UHID_CREATE2,
/* choose vendor+product IDs that will be rejected by hid_add_device() */
.u.create2 = { .rd_size = 1, .vendor = 0x07c0, .product = 0x1500 }
};
if (write(dev, &ev_create, sizeof(ev_create)) <= 0)
err(1, "write CREATE");
while (1) {
struct uhid_event ev_input = {
.type = UHID_INPUT2,
.u.input2 = {
.data = { 0xff },
.size = 1
}
};
int res = write(dev, &ev_input, sizeof(ev_input));
if (res == -1 && errno == EINVAL)
break;
}
}
}
It results in a splat like this:
BUG: KASAN: use-after-free in __lock_acquire+0x3eb9/0x5550
Read of size 8 at addr ffff88800c2218f8 by task uhid_uaf/588
CPU: 1 PID: 588 Comm: uhid_uaf Not tainted 5.16.0-08301-gfb3b0673b7d5 #886
[...]
Call Trace:
[...]
lock_acquire+0x1b9/0x4e0
_raw_spin_lock_irqsave+0x3e/0x60
down_trylock+0x13/0x70
hid_input_report+0x3d/0x500
uhid_char_write+0x210/0xdb0
vfs_write+0x1c7/0x920
ksys_write+0x176/0x1d0
do_syscall_64+0x43/0x90
entry_SYSCALL_64_after_hwframe+0x44/0xae
[...]
drivers/hid/uhid.c | 29 +++++++++++++++++++++++++----
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8fe3efcb8327..fc06d8bb42e0 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -28,11 +28,22 @@
struct uhid_device {
struct mutex devlock;
+
+ /* This flag tracks whether the HID device is usable for commands from
+ * userspace. The flag is already set before hid_add_device(), which
+ * runs in workqueue context, to allow hid_add_device() to communicate
+ * with userspace.
+ * However, if hid_add_device() fails, the flag is cleared without
+ * holding devlock.
+ * We guarantee that if @running changes from true to false while you're
+ * holding @devlock, it's still fine to access @hid.
+ */
bool running;
__u8 *rd_data;
uint rd_size;
+ /* When this is NULL, userspace may use UHID_CREATE/UHID_CREATE2. */
struct hid_device *hid;
struct uhid_event input_buf;
@@ -63,9 +74,18 @@ static void uhid_device_add_worker(struct work_struct *work)
if (ret) {
hid_err(uhid->hid, "Cannot register HID device: error %d\n", ret);
- hid_destroy_device(uhid->hid);
- uhid->hid = NULL;
+ /* We used to call hid_destroy_device() here, but that's really
+ * messy to get right because we have to coordinate with
+ * concurrent writes from userspace that might be in the middle
+ * of using uhid->hid.
+ * Just leave uhid->hid as-is for now, and clean it up when
+ * userspace tries to close or reinitialize the uhid instance.
+ *
+ * However, we do have to clear the ->running flag and do a
+ * wakeup to make sure userspace knows that the device is gone.
+ */
uhid->running = false;
+ wake_up_interruptible(&uhid->report_wait);
}
}
@@ -474,7 +494,7 @@ static int uhid_dev_create2(struct uhid_device *uhid,
void *rd_data;
int ret;
- if (uhid->running)
+ if (uhid->hid)
return -EALREADY;
rd_size = ev->u.create2.rd_size;
@@ -556,7 +576,7 @@ static int uhid_dev_create(struct uhid_device *uhid,
static int uhid_dev_destroy(struct uhid_device *uhid)
{
- if (!uhid->running)
+ if (!uhid->hid)
return -EINVAL;
uhid->running = false;
@@ -565,6 +585,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
cancel_work_sync(&uhid->worker);
hid_destroy_device(uhid->hid);
+ uhid->hid = NULL;
kfree(uhid->rd_data);
return 0;
base-commit: fb3b0673b7d5b477ed104949450cd511337ba3c6
--
2.34.1.703.g22d0c6ccf7-goog
The flag uhid->running can be set to false by uhid_device_add_worker()
without holding the uhid->devlock. Mark all reads/writes of the flag
that might race with READ_ONCE()/WRITE_ONCE() for clarity and
correctness.
Signed-off-by: Jann Horn <[email protected]>
---
Notes:
This is just some cleanup to make it clearer what's going on and avoid UB.
If you don't want to take this patch, feel free to just take patch 1/2.
drivers/hid/uhid.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fc06d8bb42e0..614adb510dbd 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -84,7 +84,7 @@ static void uhid_device_add_worker(struct work_struct *work)
* However, we do have to clear the ->running flag and do a
* wakeup to make sure userspace knows that the device is gone.
*/
- uhid->running = false;
+ WRITE_ONCE(uhid->running, false);
wake_up_interruptible(&uhid->report_wait);
}
}
@@ -194,9 +194,9 @@ static int __uhid_report_queue_and_wait(struct uhid_device *uhid,
spin_unlock_irqrestore(&uhid->qlock, flags);
ret = wait_event_interruptible_timeout(uhid->report_wait,
- !uhid->report_running || !uhid->running,
+ !uhid->report_running || !READ_ONCE(uhid->running),
5 * HZ);
- if (!ret || !uhid->running || uhid->report_running)
+ if (!ret || !READ_ONCE(uhid->running) || uhid->report_running)
ret = -EIO;
else if (ret < 0)
ret = -ERESTARTSYS;
@@ -237,7 +237,7 @@ static int uhid_hid_get_report(struct hid_device *hid, unsigned char rnum,
struct uhid_event *ev;
int ret;
- if (!uhid->running)
+ if (!READ_ONCE(uhid->running))
return -EIO;
ev = kzalloc(sizeof(*ev), GFP_KERNEL);
@@ -279,7 +279,7 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum,
struct uhid_event *ev;
int ret;
- if (!uhid->running || count > UHID_DATA_MAX)
+ if (!READ_ONCE(uhid->running) || count > UHID_DATA_MAX)
return -EIO;
ev = kzalloc(sizeof(*ev), GFP_KERNEL);
@@ -579,7 +579,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
if (!uhid->hid)
return -EINVAL;
- uhid->running = false;
+ WRITE_ONCE(uhid->running, false);
wake_up_interruptible(&uhid->report_wait);
cancel_work_sync(&uhid->worker);
@@ -593,7 +593,7 @@ static int uhid_dev_destroy(struct uhid_device *uhid)
static int uhid_dev_input(struct uhid_device *uhid, struct uhid_event *ev)
{
- if (!uhid->running)
+ if (!READ_ONCE(uhid->running))
return -EINVAL;
hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input.data,
@@ -604,7 +604,7 @@ static int uhid_dev_input(struct uhid_device *uhid, struct uhid_event *ev)
static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
{
- if (!uhid->running)
+ if (!READ_ONCE(uhid->running))
return -EINVAL;
hid_input_report(uhid->hid, HID_INPUT_REPORT, ev->u.input2.data,
@@ -616,7 +616,7 @@ static int uhid_dev_input2(struct uhid_device *uhid, struct uhid_event *ev)
static int uhid_dev_get_report_reply(struct uhid_device *uhid,
struct uhid_event *ev)
{
- if (!uhid->running)
+ if (!READ_ONCE(uhid->running))
return -EINVAL;
uhid_report_wake_up(uhid, ev->u.get_report_reply.id, ev);
@@ -626,7 +626,7 @@ static int uhid_dev_get_report_reply(struct uhid_device *uhid,
static int uhid_dev_set_report_reply(struct uhid_device *uhid,
struct uhid_event *ev)
{
- if (!uhid->running)
+ if (!READ_ONCE(uhid->running))
return -EINVAL;
uhid_report_wake_up(uhid, ev->u.set_report_reply.id, ev);
--
2.34.1.703.g22d0c6ccf7-goog
On Fri, 14 Jan 2022, Jann Horn wrote:
> uhid has to run hid_add_device() from workqueue context while allowing
> parallel use of the userspace API (which is protected with ->devlock).
> But hid_add_device() can fail. Currently, that is handled by immediately
> destroying the associated HID device, without using ->devlock - but if
> there are concurrent requests from userspace, that's wrong and leads to
> NULL dereferences and/or memory corruption (via use-after-free).
>
> Fix it by leaving the HID device as-is in the worker. We can clean it up
> later, either in the UHID_DESTROY command handler or in the ->release()
> handler.
>
> Cc: [email protected]
> Fixes: 67f8ecc550b5 ("HID: uhid: fix timeout when probe races with IO")
> Signed-off-by: Jann Horn <[email protected]>
I've queued both patches for 5.17, thanks a lot for fixing this.
--
Jiri Kosina
SUSE Labs