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

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

[Yousef Alhouseen]

hiddev_disconnect() drops existancelock before freeing the hiddev state,
but a waiting final file release can run as soon as the mutex becomes
available. On PREEMPT_RT, that waiter may free hiddev while the disconnect
thread is still executing the mutex slow-unlock path, causing a
use-after-free in the mutex implementation.

Give the connection and each open file an explicit reference. Drop each
reference only after its existancelock critical section has completed, so
the state cannot be freed from the other unlock path.

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>
---
 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] HID: hiddev: keep state alive through disconnect unlock

[Hillf Danton]

On Sun, 28 Jun 2026 11:32:45 +0200 Yousef Alhouseen wrote:
> hiddev_disconnect() drops existancelock before freeing the hiddev state,
> but a waiting final file release can run as soon as the mutex becomes
> available. On PREEMPT_RT, that waiter may free hiddev while the disconnect
> thread is still executing the mutex slow-unlock path, causing a
> use-after-free in the mutex implementation.
> 
The root cause is not specified as clearly as expected.

	hiddev_disconnect()	hiddev_release()
	---			---
	mutex_lock(&hiddev->existancelock);
	hiddev->exist = 0;

	if (hiddev->open) {
		hid_hw_close(hiddev->hid);
		wake_up_interruptible(&hiddev->wait);
		mutex_unlock(&hiddev->existancelock);
		__mutex_unlock_slowpath()
		raw_spin_lock_irqsave()   //UAF

				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); // free mem
						vfree(list);
						return 0;
					}
				}
				mutex_unlock(&list->hiddev->existancelock);

Given the syzbot report, uaf occured upon acquiring the raw spinlock in
__mutex_unlock_slowpath(), but no mutex waiter could be waken up without
the raw spinlock held, thus the report sounds false positive.

> Give the connection and each open file an explicit reference. Drop each
> reference only after its existancelock critical section has completed, so
> the state cannot be freed from the other unlock path.
> 
> 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>
> ---
>  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] HID: hiddev: keep state alive through disconnect unlock

[Yousef Alhouseen]

The missing interleaving is that mutex_unlock() clears the owner
before taking wait_lock. A contender already spinning on existancelock
can acquire it after that owner clear, reach the final-release path,
and free hiddev before the disconnecting task reaches
raw_spin_lock_irqsave(&lock->wait_lock, ...). It does not need to be
woken by the disconnecting task.

That is also why mutex_unlock() explicitly requires the containing
object to remain alive until the call returns. I will make this
interleaving explicit in v2.

Thanks,
Yousef

On Mon, 29 Jun 2026 06:26:15 +0800, Hillf Danton <hdanton@sina.com> wrote:
> On Sun, 28 Jun 2026 11:32:45 +0200 Yousef Alhouseen wrote:
> > hiddev_disconnect() drops existancelock before freeing the hiddev state,
> > but a waiting final file release can run as soon as the mutex becomes
> > available. On PREEMPT_RT, that waiter may free hiddev while the disconnect
> > thread is still executing the mutex slow-unlock path, causing a
> > use-after-free in the mutex implementation.
> >
> The root cause is not specified as clearly as expected.
>
> hiddev_disconnect() hiddev_release()
> --- ---
> mutex_lock(&hiddev->existancelock);
> hiddev->exist = 0;
>
> if (hiddev->open) {
> hid_hw_close(hiddev->hid);
> wake_up_interruptible(&hiddev->wait);
> mutex_unlock(&hiddev->existancelock);
> __mutex_unlock_slowpath()
> raw_spin_lock_irqsave() //UAF
>
> 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); // free mem
> vfree(list);
> return 0;
> }
> }
> mutex_unlock(&list->hiddev->existancelock);
>
> Given the syzbot report, uaf occured upon acquiring the raw spinlock in
> __mutex_unlock_slowpath(), but no mutex waiter could be waken up without
> the raw spinlock held, thus the report sounds false positive.
>
> > Give the connection and each open file an explicit reference. Drop each
> > reference only after its existancelock critical section has completed, so
> > the state cannot be freed from the other unlock path.
> >
> > 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>
> > ---
> > 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] HID: hiddev: keep state alive through disconnect unlock

[Hillf Danton]

[ feel free not to send top reply ]
On Mon, 29 Jun 2026 08:57:02 -0500 Yousef Alhouseen wrote:
> The missing interleaving is that mutex_unlock() clears the owner
> before taking wait_lock. A contender already spinning on existancelock
> can acquire it after that owner clear, reach the final-release path,
> and free hiddev before the disconnecting task reaches
> raw_spin_lock_irqsave(&lock->wait_lock, ...). It does not need to be
> woken by the disconnecting task.
> 
In __mutex_unlock_slowpath() the wait_lock is taken because of MUTEX_FLAG_WAITERS
after clearing the lock owner, so it is bug in the mutex backyard instead if
mutex is freed without pending waiters handled.