Re: [PATCH] HID: hiddev: Use kref to track struct hiddev lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Use-After-Free in hiddev_send_event due to race between hid_disconnect and usbhid_irq_in.
--

commit a4056f7ab6df2200de09a38f314eabfb8dd4a066
Author: Heitor Alves de Siqueira <halves@igalia.com>

HID: hiddev: Use kref to track struct hiddev lifetime

This commit fixes a race between hiddev_disconnect() and hiddev_release()
by introducing a kref in struct hiddev. It moves the deallocation into a
dedicated release callback, ensuring the structure is only freed after both
functions are complete.

Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4

> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 6378801b22c6d..1f2802920beee 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c

[ ... ]

> @@ -933,13 +947,11 @@ void hiddev_disconnect(struct hid_device *hid)
>  
>  	mutex_lock(&hiddev->existancelock);
>  	hiddev->exist = 0;
> -
>  	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);
> +
> +	kref_put(&hiddev->kref, hiddev_kref_free);

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a use-after-free
race condition when freeing struct hiddev while URBs are still active?

When a USB HID device is disconnected, usbhid_disconnect() eventually calls
hid_hw_stop(), which tears down the high-level structures before stopping
the low-level driver:

drivers/hid/hid-core.c:hid_hw_stop() {
	hid_disconnect(hdev);
	hdev->ll_driver->stop(hdev);
}

Because hid_disconnect() calls hiddev_disconnect() (which drops the kref
here), if no userspace process holds the file open, struct hiddev will be
freed immediately.

Since the input URBs have not yet been killed by hdev->ll_driver->stop(),
can an URB complete concurrently on another CPU? 

If the URB completes, the completion handler usbhid_irq_in() propagates to
hid_input_report() -> hid_input_field() -> hiddev_send_event(), which then
dereferences the freed hiddev pointer:

drivers/hid/usbhid/hiddev.c:hiddev_send_event() {
	struct hiddev *hiddev = hid->hiddev;
	struct hiddev_list *list;
	unsigned long flags;

	spin_lock_irqsave(&hiddev->list_lock, flags);

Would this cause a use-after-free panic in interrupt context when it tries
to acquire list_lock?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-hiddev_kref-v1-1-cd240c95423f@igalia.com?part=1