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

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

[Yousef Alhouseen]

mutex_unlock() clears the mutex owner before taking its wait lock. A
thread spinning in the final hiddev file release can acquire
existancelock after hiddev_disconnect() clears the owner, then free
hiddev before the disconnecting thread reaches the mutex wait lock. This
causes a use-after-free in the mutex slow unlock path.

Give the connection and each open file an explicit reference. Drop each
reference only after its existancelock critical section has completed,
so neither unlock path can free the mutex while the other is still using
it.

Fixes: 079034073faf ("HID: hiddev cleanup -- handle all error conditions properly")
Reported-by: syzbot+563191a4939ddbfe73d4@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>
---
Changes in v2:
- Explain the mutex owner-clear/spinning-contender race in the commit log.
- No code changes.

 drivers/hid/usbhid/hiddev.c | 37 +++++++++++++++++++------------------
 include/linux/hiddev.h      |  2 ++
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 6378801b22c6..21396481995b 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -46,6 +46,12 @@ struct hiddev_list {
 	struct mutex thread_lock;
 };
 
+static void hiddev_put(struct hiddev *hiddev)
+{
+	if (refcount_dec_and_test(&hiddev->refcount))
+		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
@@ -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);
 	}
 
-	mutex_unlock(&list->hiddev->existancelock);
+	mutex_unlock(&hiddev->existancelock);
+	hiddev_put(hiddev);
 	vfree(list);
 
 	return 0;
@@ -270,6 +271,7 @@ static int __hiddev_open(struct hiddev *hiddev, struct file *file)
 	spin_unlock_irq(&hiddev->list_lock);
 
 	file->private_data = list;
+	refcount_inc(&hiddev->refcount);
 
 	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;
@@ -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);
 }
diff --git a/include/linux/hiddev.h b/include/linux/hiddev.h
index 2164c03d2c72..8e9f8a33e359 100644
--- a/include/linux/hiddev.h
+++ b/include/linux/hiddev.h
@@ -13,6 +13,7 @@
 #ifndef _HIDDEV_H
 #define _HIDDEV_H
 
+#include <linux/refcount.h>
 #include <uapi/linux/hiddev.h>
 
 
@@ -24,6 +25,7 @@ struct hiddev {
 	int minor;
 	int exist;
 	int open;
+	refcount_t refcount;
 	struct mutex existancelock;
 	wait_queue_head_t wait;
 	struct hid_device *hid;
-- 
2.54.0

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

[sashiko-bot]

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