HID: uhid: Fix worker destroying device without any protection

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: 4bf8c533c8e1 ("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>

diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index 8fe3efcb832715c4a978625387dc0bb0ea5b00a2..fc06d8bb42e0fe94e75167fe360c600283f80a4d 100644 (file)
--- 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;