Re: [PATCH] HID: roccat: free buffered reports when destroying device

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

Pre-existing issues:
- [High] Concurrent execution of `roccat_disconnect()` and `roccat_release()` leads to Use-After-Free and Double-Free vulnerabilities.
- [High] `roccat_release()` fails to free disconnected open devices due to a faulty array lookup, leaking memory and corrupting state if the minor number is reused.
- [High] Incoming HID events during or immediately after device removal trigger Use-After-Free, Double-Free, and panics due to missing teardown synchronization.
- [High] `roccat_report_event()` acquires sleeping locks in an atomic hardware interrupt context.
- [High] `roccat_connect()` publishes the device globally before initializing its mutexes and lists, leading to uninitialized use crashes.
--

--- Patch [1]: [PATCH] HID: roccat: free buffered reports when destroying device ---
commit dad9381303bffdf0e85a009b8168cbabdcb74643
Author: Xu Rao <[email protected]>

HID: roccat: free buffered reports when destroying device

This commit adds a destructor that frees every buffered report before freeing
the device. It uses this new destructor in both paths that destroy a
registered device to prevent leaking up to ROCCAT_CBUF_SIZE report buffers
per device.

Cc: [email protected]

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index d6fff53d4ee7a..4f15eb9510396 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -70,6 +70,15 @@ static struct roccat_device *devices[ROCCAT_MAX_DEVICES];
>  /* protects modifications of devices array */
>  static DEFINE_MUTEX(devices_lock);
>  
> +static void roccat_free_device(struct roccat_device *device)
> +{
> +	int i;
> +
> +	for (i = 0; i < ROCCAT_CBUF_SIZE; i++)
> +		kfree(device->cbuf[i].value);

[Severity: High]
This is a pre-existing issue, but does this lockless iteration race with the
interrupt handler?

If a sub-driver like hid-roccat-kone triggers roccat_disconnect() and frees
its state before shutting down hardware events, HID interrupts can continue
firing during teardown.

Could this lockless kfree() race with an active interrupt handler modifying
the buffer, resulting in an immediate double-free of the buffer memory?

> +	kfree(device);
> +}
> +

[ ... ]

> @@ -226,7 +235,7 @@ static int roccat_release(struct inode *inode, struct file *file)
>  			hid_hw_power(device->hid, PM_HINT_NORMAL);
>  			hid_hw_close(device->hid);
>  		} else {
> -			kfree(device);
> +			roccat_free_device(device);
>  		}
>  	}

[Severity: High]
This is a pre-existing issue, but does roccat_release() fail to free
disconnected open devices due to the device lookup earlier in this function?

When an open device is unplugged, roccat_disconnect() sets
devices[minor] = NULL.

Later, when the file descriptor is closed, roccat_release() looks up the
device:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    device = devices[minor];
    if (!device) {
        mutex_unlock(&devices_lock);
        pr_emerg("roccat device with minor %d doesn't exist\n", minor);
        return -ENODEV;
    }
    ...
}

Since it is NULL, won't the function return -ENODEV early and skip this
cleanup logic entirely?

Furthermore, if a new Roccat device is plugged in before the descriptor is
closed, it reuses the same minor number. Does this mean roccat_release() will
fetch the new device, incorrectly decrement its open count, and remove the
old reader from the new device's list?

[ ... ]

> @@ -374,7 +383,7 @@ void roccat_disconnect(int minor)
>  		hid_hw_close(device->hid);
>  		wake_up_interruptible(&device->wait);
>  	} else {
> -		kfree(device);
> +		roccat_free_device(device);
>  	}
>  }
>  EXPORT_SYMBOL_GPL(roccat_disconnect);

[Severity: High]
This is a pre-existing issue, but can roccat_disconnect() and
roccat_release() race to cause a use-after-free and double-free?

In roccat_disconnect(), the global devices_lock is dropped before checking
device->open:

drivers/hid/hid-roccat.c:roccat_disconnect() {
    ...
    device->exist = 0;
    device_destroy(device->dev->class, MKDEV(roccat_major, minor));
    
    mutex_lock(&devices_lock);
    devices[minor] = NULL;
    mutex_unlock(&devices_lock);
    
    if (device->open) {
    ...
}

If a user closes an open file descriptor exactly when the device is being
hot-unplugged, could roccat_release() acquire the lock during this window,
see exist == 0, and free the device?

If so, when roccat_disconnect() continues, it would access the freed device
to check device->open, and potentially call roccat_free_device() a second
time.

[Severity: High]
This is a pre-existing issue, but does roccat_report_event() acquire
sleeping locks in an atomic context?

This function runs from the HID raw_event callback in atomic/softirq context.
However, it attempts to acquire mutexes:

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    mutex_lock(&device->readers_lock);
    mutex_lock(&device->cbuf_lock);
    ...
}

Since mutexes can sleep, does this cause scheduling while atomic panics when
hardware events arrive?

[Severity: High]
This is a pre-existing issue, but does roccat_connect() publish the device
globally before its fundamental structures are initialized?

The device pointer is made accessible to interrupt handlers early:

drivers/hid/hid-roccat.c:roccat_connect() {
    ...
    if (minor < ROCCAT_MAX_DEVICES) {
        devices[minor] = device;
    ...
    INIT_LIST_HEAD(&device->readers);
    mutex_init(&device->readers_lock);
    ...
}

Because hid_hw_start() is called by sub-drivers prior to roccat_connect(),
hardware events are already active. If an interrupt fires during this narrow
window, could roccat_report_event() fetch the device and attempt to lock
uninitialized mutexes or iterate an uninitialized readers list?

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