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@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://syzkaller.appspot.com/bug?extid=563191a4939ddbfe73d4
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Yousef Alhouseen <alhouseenyousef@xxxxxxxxx>
> > ---
> > 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