Re: [PATCH 1/2] HID: roccat: fix locking in roccat_connect() and roccat_disconnect()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Lockless access to `devices` array in `roccat_report_event()` races with `roccat_disconnect()`, leading to a NULL pointer dereference.
- [High] `roccat_report_event()` incorrectly calls sleeping `mutex_lock()` functions inside an atomic interrupt context.
- [High] Tear-down ordering violation in hardware-specific sub-drivers frees private data before calling `hid_hw_stop()`, leading to a Use-After-Free in `raw_event`.
--

commit 703610255dcd5d9ce2d448d2494e1361c5869fcf
Author: Dmitry Antipov <dmantipov@yandex.ru>

HID: roccat: fix locking in roccat_connect() and roccat_disconnect()
    
Extend critical section in roccat_connect() to ensure that partially
initialized 'struct roccat_device' is never exposed in 'devices' list,
and do the same in roccat_disconnect() to avoid racy 'devices' access
against roccat_release().

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 4f15eb9510396..5deb6da8d4f74 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -369,15 +368,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;
> -	mutex_unlock(&devices_lock);
[Severity: High]
This is a pre-existing issue, but does setting `devices[minor] = NULL` here
without stopping the hardware polling first create a race condition?

When a Roccat device is unplugged and has no open references, this sets
`devices[minor] = NULL` and frees the device. Because this occurs before
`hid_hw_stop()` is called in the sub-drivers, hardware polling is still active.

If an interrupt fires during this window, could `roccat_report_event()`
locklessly fetch the device and encounter a NULL pointer dereference?

drivers/hid/hid-roccat.c:roccat_report_event() {
	...
	device = devices[minor];
	new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
	...
}

[Severity: High]
This is a pre-existing issue, but is it possible for `roccat_report_event()`
to attempt to acquire sleeping locks in an atomic context?

`roccat_report_event()` is called synchronously from HID `raw_event` handlers
which execute in an atomic softirq context. While it correctly uses
`GFP_ATOMIC` for its allocation, it seems to call `mutex_lock()` immediately
after:

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

Can this lead to a kernel panic or a 'scheduling while atomic' bug?

[Severity: High]
This is a pre-existing issue, but is there a tear-down ordering violation in
hardware-specific sub-drivers like `hid-roccat-kone.c`?

During driver unbind or device unplug, the `remove` function calls
`kone_remove_specials()` which frees the private data before calling
`hid_hw_stop()`:

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);
	...
}

Because hardware polling isn't stopped before freeing, incoming reports could
execute the `raw_event` handler, which fetches the freed pointer:

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))
	...
}

Could this result in a Use-After-Free memory corruption if `kone` is
dereferenced for reads or writes here?

>  
>  	if (device->open) {
>  		hid_hw_close(device->hid);
> @@ -385,6 +381,8 @@ void roccat_disconnect(int minor)
>  	} else {
>  		roccat_free_device(device);
>  	}
> +
> +	mutex_unlock(&devices_lock);
>  }
>  EXPORT_SYMBOL_GPL(roccat_disconnect);

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