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

[Hillf Danton <hdanton@sina.com>]

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