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

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

[Heitor Alves de Siqueira]

If a USB HID device is disconnected while userspace still holds the
hiddev node open, hiddev_disconnect() and hiddev_release() can race on
the embedded existancelock mutex. Syzbot has triggered this with kfree()
happening during the mutex slow path.

Fix by introducing a kref in struct hiddev, and moving kfree() into a
dedicated release callback. This way, struct hiddev will only be freed
after both hiddev_release() and hiddev_disconnect() are done.

Fixes: 6cb4b040795c ("HID: hiddev: fix race between hiddev_disconnect and hiddev_release")
Reported-by: syzbot+563191a4939ddbfe73d4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
Signed-off-by: Heitor Alves de Siqueira <halves@igalia.com>
---
 drivers/hid/usbhid/hiddev.c | 36 ++++++++++++++++++++++++------------
 include/linux/hiddev.h      |  1 +
 2 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 6378801b22c6..1f2802920bee 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -46,6 +46,13 @@ struct hiddev_list {
 	struct mutex thread_lock;
 };
 
+static void hiddev_kref_free(struct kref *kref)
+{
+	struct hiddev *hiddev = container_of(kref, struct hiddev, kref);
+
+	kfree(hiddev);
+}
+
 /*
  * Find a report, given the report's type and ID.  The ID can be specified
  * indirectly by REPORT_ID_FIRST (which returns the first report of the given
@@ -227,15 +234,11 @@ static int hiddev_release(struct inode * inode, struct file * file)
 		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_unlock(&list->hiddev->existancelock);
+
+	kref_put(&list->hiddev->kref, hiddev_kref_free);
 	vfree(list);
 
 	return 0;
@@ -298,10 +301,20 @@ static int hiddev_open(struct inode *inode, struct file *file)
 	hid = usb_get_intfdata(intf);
 	hiddev = hid->hiddev;
 
+	/*
+	 * kref_get_unless_zero() checks if we've already dropped
+	 * the last reference; bail out early in this case
+	 */
+	if (!hiddev || !kref_get_unless_zero(&hiddev->kref))
+		return -ENODEV;
+
 	mutex_lock(&hiddev->existancelock);
 	res = hiddev->exist ? __hiddev_open(hiddev, file) : -ENODEV;
 	mutex_unlock(&hiddev->existancelock);
 
+	if (res < 0)
+		kref_put(&hiddev->kref, hiddev_kref_free);
+
 	return res;
 }
 
@@ -893,6 +906,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (!(hiddev = kzalloc_obj(struct hiddev)))
 		return -ENOMEM;
 
+	kref_init(&hiddev->kref);
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
 	spin_lock_init(&hiddev->list_lock);
@@ -904,7 +918,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	if (retval) {
 		hid_err(hid, "Not able to get a minor for this device\n");
 		hid->hiddev = NULL;
-		kfree(hiddev);
+		kref_put(&hiddev->kref, hiddev_kref_free);
 		return retval;
 	}
 
@@ -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);
 }
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index 2164c03d2c72..1613c9c37ab7 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -24,6 +24,7 @@ struct hiddev {
 	int minor;
 	int exist;
 	int open;
+	struct kref kref;
 	struct mutex existancelock;
 	wait_queue_head_t wait;
 	struct hid_device *hid;

---
base-commit: e43ffb69e0438cddd72aaa30898b4dc446f664f8
change-id: 20260603-hiddev_kref-52f5507d4e42

Best regards,
--  
Heitor Alves de Siqueira <halves@igalia.com>

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

[sashiko-bot]

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

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

[Dmitry Torokhov]

Hi Heitor,

On Mon, Jun 08, 2026 at 01:33:03PM -0300, Heitor Alves de Siqueira wrote:
> If a USB HID device is disconnected while userspace still holds the
> hiddev node open, hiddev_disconnect() and hiddev_release() can race on
> the embedded existancelock mutex. Syzbot has triggered this with kfree()
> happening during the mutex slow path.
> 
> Fix by introducing a kref in struct hiddev, and moving kfree() into a
> dedicated release callback. This way, struct hiddev will only be freed
> after both hiddev_release() and hiddev_disconnect() are done.

This looks like a common issue with usb_register_dev() that does not
allow tying the lifetime of the created device, lifetime of user of the
created device, and userspace accessing it. Ideally the class device
would be embedded into struct hiddev, and tie its lifetime with lifetime
of the chardev associated with it and userspace accessors using it. tie
its lifetime with lifetime of the chardev associated with it and
userspace accessors using it. See cdev_device_add() and how it is being
used by multiple subsystems and how they handle class devices. 

Thanks.

-- 
Dmitry