From: Jann Horn jannh@google.com
commit 4ea5763fb79ed89b3bdad455ebf3f33416a81624 upstream.
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: stable@vger.kernel.org Fixes: 67f8ecc550b5 ("HID: uhid: fix timeout when probe races with IO") Signed-off-by: Jann Horn jannh@google.com Signed-off-by: Jiri Kosina jkosina@suse.cz Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- drivers/hid/uhid.c | 29 +++++++++++++++++++++++++---- 1 file changed, 25 insertions(+), 4 deletions(-)
--- a/drivers/hid/uhid.c +++ b/drivers/hid/uhid.c @@ -33,11 +33,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;
@@ -68,9 +79,18 @@ static void uhid_device_add_worker(struc 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); } }
@@ -479,7 +499,7 @@ static int uhid_dev_create2(struct uhid_ void *rd_data; int ret;
- if (uhid->running) + if (uhid->hid) return -EALREADY;
rd_size = ev->u.create2.rd_size; @@ -560,7 +580,7 @@ static int uhid_dev_create(struct uhid_d
static int uhid_dev_destroy(struct uhid_device *uhid) { - if (!uhid->running) + if (!uhid->hid) return -EINVAL;
uhid->running = false; @@ -569,6 +589,7 @@ static int uhid_dev_destroy(struct uhid_ cancel_work_sync(&uhid->worker);
hid_destroy_device(uhid->hid); + uhid->hid = NULL; kfree(uhid->rd_data);
return 0;