lkml.org 
[lkml]   [2022]   [Jan]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 5.16 0004/1039] HID: uhid: Fix worker destroying device without any protection
    Date
    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
    @@ -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(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);
    }
    }

    @@ -474,7 +494,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;
    @@ -556,7 +576,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;
    @@ -565,6 +585,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;

    \
     
     \ /
      Last update: 2022-01-24 23:09    [W:3.970 / U:1.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site