Re: [PATCH v2 4/4] HID: roccat: use kref to manage device instances

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] A TOCTOU race condition between `roccat_release` and `roccat_open` causes a new reader's device to be incorrectly powered down and closed.

Pre-existing issues:
- [High] Data path UAF and NULL pointer dereference in `roccat_report_event` due to lockless device access.
- [High] Custom Roccat drivers violate HID subsystem lifecycle rules by freeing their private `drvdata` before stopping the hardware, causing a UAF in their `raw_event` handlers.
- [High] A race condition between `roccat_release` and `roccat_disconnect` causes `hid_hw_close()` and `hid_hw_power()` to be skipped entirely, permanently leaking `ll_open_count` and power references.
--

commit 0e2654cfadc18d56d9b77ac333344436bcca55c7
Author: Dmitry Antipov <dmantipov@yandex.ru>

HID: roccat: use kref to manage device instances

Use kref to manage 'struct roccat_device' instances and fix
UaF-triggering race between roccat_open()/roccat_release()
and roccat_connect()/roccat_disconnect() pairs.

Reported-by: syzbot+d632e93ffcd1452bc61e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e
Link: https://sashiko.dev/#/patchset/20260902094551.200587-1-dmantipov@yandex.ru?part=2

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index d1df4ec34a1e6..32927b20bcfe0 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -227,9 +231,8 @@ static int roccat_release(struct inode *inode, struct file *file)
>  		if (device->exist) {
>  			hid_hw_power(device->hid, PM_HINT_NORMAL);
>  			hid_hw_close(device->hid);
> -		} else {
> -			roccat_free_device(device);
>  		}
> +		kref_put(&device->ref, roccat_free_device);
>  	}
>  
>  	mutex_unlock(&devices_lock);

[Severity: High]
Can a newly opened reader's device be incorrectly closed and powered down due
to a TOCTOU race?

Looking at roccat_release(), the open status is determined early while holding
device->readers_lock. A concurrent roccat_open() can then observe the empty
list, power on the device, and add its reader node.

When roccat_release() later acquires devices_lock, it acts on the stale open
variable (which is false):

CPU1: roccat_release()
    mutex_lock(&device->readers_lock);
    open = !list_empty(&device->readers); // false
    mutex_unlock(&device->readers_lock);

CPU2: roccat_open()
    mutex_lock(&devices_lock);
    mutex_lock(&device->readers_lock);
    // list is empty, power on and open hardware
    hid_hw_power(device->hid, PM_HINT_FULLON);
    ...
    mutex_unlock(&device->readers_lock);
    mutex_unlock(&devices_lock);

CPU1: roccat_release() (resuming)
    mutex_lock(&devices_lock);
    if (!open) {
        hid_hw_power(device->hid, PM_HINT_NORMAL);
        hid_hw_close(device->hid);
    }

Does this clobber the concurrent open and leave the newly connected reader with
a powered down, closed hardware state?

> @@ -362,6 +366,8 @@ void roccat_disconnect(int minor)
>  
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
> +	if (!device)
> +		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but does roccat_report_event() risk
a NULL pointer dereference or use-after-free because it accesses devices[minor]
locklessly?

When the device is unplugged, roccat_disconnect() sets devices[minor] = NULL
and drops the kref. Since the transport hardware hasn't been stopped yet (due
to the delayed hid_hw_close), incoming USB events can concurrently trigger
kone_raw_event(), which calls into roccat_report_event():

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    device = devices[minor];

    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
    ...
}

If devices[minor] is read as NULL here, accessing device->report_size will
trigger a panic. If it is read right before kref_put frees it, does this result
in a use-after-free, defeating the intent of the kref fix?

> @@ -376,10 +382,10 @@ void roccat_disconnect(int minor)
>  	if (open) {
>  		hid_hw_close(device->hid);
>  		wake_up_interruptible(&device->wait);
> -	} else {
> -		roccat_free_device(device);
>  	}
>  
> +	kref_put(&device->ref, roccat_free_device);
> +out:
>  	mutex_unlock(&devices_lock);
>  }

[Severity: High]
This is a pre-existing issue, but can a race between roccat_release() and
roccat_disconnect() permanently leak the ll_open_count and power references by
skipping hid_hw_close() entirely?

If roccat_disconnect() sets device->exist = 0 concurrently with the last reader
calling roccat_release(), both paths appear to bypass the mandatory cleanup:

CPU1: roccat_disconnect()
    device->exist = 0;
    // open is false because reader is removing itself
    if (open) { ... } // skipped

CPU2: roccat_release()
    // open is false
    if (!open) {
        if (device->exist) {
            // skipped because exist is 0
            hid_hw_close(device->hid);
        }
    }

Does this leave the hardware permanently active and prevent proper unbinding?


While reviewing the UAF fixes in the roccat driver, I noticed another
pre-existing UAF in the kone driver's lifecycle handling:

[Severity: High]
This isn't a bug introduced by this patch, but does the kone driver violate
HID lifecycle rules by freeing its private driver data before stopping the
hardware?

Because kfree(hid_get_drvdata(hdev)) is called before hid_hw_stop(hdev),
incoming USB events can still be delivered during this window.

drivers/hid/hid-roccat-kone.c:kone_remove_specials() {
    ...
        kfree(hid_get_drvdata(hdev));
    ...
}

drivers/hid/hid-roccat-kone.c:kone_remove() {
    kone_remove_specials(hdev);
    hid_hw_stop(hdev);
}

If an interrupt arrives before hid_hw_stop(), kone_raw_event() accesses the
freed kone structure:

drivers/hid/hid-roccat-kone.c:kone_raw_event() {
    struct kone_device *kone = hid_get_drvdata(hdev);
    struct kone_mouse_event *event = (struct kone_mouse_event *)data;
    ...
    if (memcmp(&kone->last_mouse_event.tilt, &event->tilt, 5))
    ...
}

Can this use-after-free be triggered locally by unplugging the device while
USB events are active?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914120240.1625442-1-dmantipov@yandex.ru?part=4