Re: [PATCH v2] HID: hiddev: keep state alive through disconnect unlock

[sashiko-bot@kernel.org]

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 <alhouseenyousef@gmail.com>

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

> 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
[ ... ]
> @@ -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;
[ ... ]
> @@ -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()?

[ ... ]
> @@ -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