Re: [PATCH] HID: roccat: fix device lifetime during disconnect

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

Pre-existing issues:
- [High] roccat_report_event() acquires mutexes while running in atomic context
- [High] roccat_report_event() dereferences NULL pointer and accesses freed memory due to concurrent disconnect
- [High] Roccat sub-drivers trigger Use-After-Free by freeing driver data before stopping HID hardware
- [High] roccat_connect() exposes uninitialized device to userspace
--

--- Patch [1]: [PATCH] HID: roccat: fix device lifetime during disconnect ---
commit 5e229d8ced35e1abadf95620f08339c5580af3c2
Author: Hongyan Xu <[email protected]>

    HID: roccat: fix device lifetime during disconnect
    
    roccat_disconnect() drops devices_lock after loading the device pointer. A
    concurrent release of the last reader can then free the device before
    disconnect continues to access it. Release also looks the device up by
    minor, so closing a file after disconnect either leaks its reader when the
    slot is NULL or operates on a replacement device if the minor was reused.
    
    Keep devices_lock held while disconnect marks and removes the device, and
    have release use the device retained by its reader. The open count then
    keeps a disconnected device alive until the final file is released.

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index d6fff53d4ee7a..4f504e7575e8c 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -360,15 +352,12 @@ void roccat_disconnect(int minor)
>  
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
> -	mutex_unlock(&devices_lock);
>  
>  	device->exist = 0; /* TODO exist maybe not needed */
>  
>  	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
>  
> -	mutex_lock(&devices_lock);
>  	devices[minor] = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but does clearing devices[minor]
here race with concurrent event reporting?

Roccat sub-drivers (like kone) call roccat_disconnect() before they stop the
hardware with hid_hw_stop(). Since events can still arrive, could
roccat_report_event() dereference a null pointer?

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

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

If it fetches devices[minor] after it is cleared here, it will crash on
device->report_size. Alternatively, could this cause a use-after-free if it
fetches the pointer just before it is cleared but accesses it after it is
freed?

> -	mutex_unlock(&devices_lock);
>  
>  	if (device->open) {
>  		hid_hw_close(device->hid);
[ ... ]

While looking at roccat_report_event(), I noticed another pre-existing issue.

[Severity: High]
This isn't a bug introduced by this patch, but does roccat_report_event()
illegally acquire sleeping locks in atomic context?

When a roccat device receives an event from the hardware, the usb completion
handler runs in softirq (atomic) context. This calls the hid driver's
raw_event callback, which calls roccat_report_event().

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
    if (!new_value)
        return -ENOMEM;

    mutex_lock(&device->readers_lock);
    mutex_lock(&device->cbuf_lock);
    ...
}

While the function correctly uses GFP_ATOMIC for allocations, it then
immediately calls mutex_lock(). Since mutexes can sleep, won't locking them
in an interrupt/atomic context cause a scheduling while atomic panic?

Looking at the device initialization in roccat_connect():

[Severity: High]
This is a pre-existing issue, but does roccat_connect() expose an
uninitialized device to userspace?

drivers/hid/hid-roccat.c:roccat_connect() {
    ...
    device->dev = device_create(klass, &hid->dev,
            MKDEV(roccat_major, minor), NULL,
            "%s%s%d", "roccat", hid->driver->name, minor);
    ...
    mutex_unlock(&devices_lock);

    init_waitqueue_head(&device->wait);
    INIT_LIST_HEAD(&device->readers);
    mutex_init(&device->readers_lock);
    mutex_init(&device->cbuf_lock);
    ...
}

The device node is published to userspace via device_create() and then the
global devices_lock is dropped. This allows userspace to open the device
node before critical state like readers_lock and cbuf_lock are initialized.

Could a concurrent roccat_open() access these uninitialized fields and cause
a crash?

Finally, regarding the sub-driver cleanup sequence:

[Severity: High]
This isn't a bug introduced by this patch, but do the roccat sub-drivers
trigger a use-after-free during removal?

In kone_remove(), the driver data is freed via kone_remove_specials() before
the hardware is stopped:

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

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

Since hid_hw_stop() is called after the memory is freed, events can still
arrive. The interrupt handler then accesses the freed struct kone_device:

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

Will this cause memory corruption or a crash when the device is disconnected?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1