[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

[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