Re: [PATCH v2] HID: hiddev: keep state alive through disconnect unlock
From: sashiko-bot@kernel.org
Date: 2026-06-29 15:54:24
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: Pre-existing issues: - [High] A race condition in `hiddev_connect()` exposes the character device before initialization is complete, allowing bypass of `HID_QUIRK_NO_INIT_REPORTS`. - [Medium] Runtime PM reference on the USB interface is leaked when the device is unbound while a hiddev file is open. - [Low] An off-by-one error in the `array_index_nospec()` clamping logic prevents reading the last valid usage index during `HIDIOCGUSAGES`. -- commit afdbfd4c2b634bf186c79d8a144039cac66bc1c8 Author: Yousef Alhouseen [off-list ref] HID: hiddev: keep state alive through disconnect unlock This commit fixes a use-after-free issue in the hiddev mutex slow unlock path. It gives the connection and each open file an explicit reference, dropping the reference only after the existancelock critical section completes. Link: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c index 6378801b22c6d..21396481995b2 100644 --- a/drivers/hid/usbhid/hiddev.c +++ b/drivers/hid/usbhid/hiddev.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -216,26 +222,21 @@ static int hiddev_fasync(int fd, struct file *file, int on) static int hiddev_release(struct inode * inode, struct file * file) { struct hiddev_list *list = file->private_data; + struct hiddev *hiddev = list->hiddev; unsigned long flags; - spin_lock_irqsave(&list->hiddev->list_lock, flags); + spin_lock_irqsave(&hiddev->list_lock, flags); list_del(&list->node); - spin_unlock_irqrestore(&list->hiddev->list_lock, flags); + spin_unlock_irqrestore(&hiddev->list_lock, flags); - mutex_lock(&list->hiddev->existancelock); - if (!--list->hiddev->open) { - if (list->hiddev->exist) { - hid_hw_close(list->hiddev->hid); - hid_hw_power(list->hiddev->hid, PM_HINT_NORMAL); - } else { - mutex_unlock(&list->hiddev->existancelock); - kfree(list->hiddev); - vfree(list); - return 0; - } + mutex_lock(&hiddev->existancelock); + if (!--hiddev->open && hiddev->exist) { + hid_hw_close(hiddev->hid); + hid_hw_power(hiddev->hid, PM_HINT_NORMAL); }
[Severity: Medium] This is a pre-existing issue, but does this create a runtime PM reference leak when the device is unbound while a hiddev file is open? If a device unbind occurs while the file remains open, hiddev_disconnect() clears hiddev->exist. When hiddev_release() is subsequently called, the code avoids the hid_hw_power() call because hiddev->exist is 0. Since the disconnect path does not call hid_hw_power() with PM_HINT_NORMAL, could this leave the power domain permanently active and defeat USB autosuspend until physical unplug?
- mutex_unlock(&list->hiddev->existancelock); + mutex_unlock(&hiddev->existancelock); + hiddev_put(hiddev); vfree(list); return 0;
[ ... ]
quoted hunk ↗ jump to hunk
@@ -897,6 +899,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force) INIT_LIST_HEAD(&hiddev->list); spin_lock_init(&hiddev->list_lock); mutex_init(&hiddev->existancelock); + refcount_set(&hiddev->refcount, 1); hid->hiddev = hiddev; hiddev->hid = hid; hiddev->exist = 1;
[Severity: High] This is a pre-existing issue, but does this create a race condition exposing the character device before initialization is complete? Shortly after this block, hiddev_connect() calls usb_register_dev() which makes the device available to userspace. However, hiddev->initialized is set later based on HID_QUIRK_NO_INIT_REPORTS. If a userspace process immediately opens the newly registered device and issues a HIDIOCGUSAGE ioctl before hiddev->initialized is set, could it bypass the quirk and improperly trigger usbhid_init_reports() in hiddev_ioctl()? [ ... ]
quoted hunk ↗ jump to hunk
@@ -937,9 +940,7 @@ void hiddev_disconnect(struct hid_device *hid) if (hiddev->open) { hid_hw_close(hiddev->hid); wake_up_interruptible(&hiddev->wait); - mutex_unlock(&hiddev->existancelock); - } else { - mutex_unlock(&hiddev->existancelock); - kfree(hiddev); } + mutex_unlock(&hiddev->existancelock); + hiddev_put(hiddev); }
[Severity: Low] This is a pre-existing issue, but is there an off-by-one error in the clamping logic within hiddev_ioctl_usage() that prevents reading the last valid usage index? When a user issues HIDIOCGUSAGES or HIDIOCSUSAGES, the code does: uref->usage_index = array_index_nospec(uref->usage_index, field->report_count - uref_multi->num_values); Because array_index_nospec() strictly requires the index to be less than the size, does an index exactly equal to the size (which is valid here) get incorrectly clamped to 0? Would this silently return the first block of usages instead of the last valid block? -- Sashiko AI review · https://sashiko.dev/#/patchset/20260629152947.13821-1-alhouseenyousef@gmail.com?part=1