Re: [PATCH v2 2/4] HID: roccat: fix device access in roccat_release()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] HID roccat sub-drivers free their private driver data before calling hid_hw_stop(), causing a Use-After-Free in the raw_event callback.
- [High] roccat_report_event() accesses devices[minor] locklessly and does not check for NULL, leading to a NULL pointer dereference during device disconnect.
- [High] roccat_report_event() acquires sleepable mutexes from an atomic interrupt context.
--

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

HID: roccat: fix device access in roccat_release()

In roccat_release(), access the device using file-specific reader data rather
than global array, thus preventing the case when original device was
disconnected and a new device has connected using the same slot indexed by minor
number.

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 5deb6da8d4f74..007778922c21a 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c

[ ... ]

> int roccat_report_event(int minor, u8 const *data)
> {
> 	struct roccat_device *device;
> 	struct roccat_reader *reader;
> 	struct roccat_report *report;
> 	uint8_t *new_value;
> 
> 	device = devices[minor];
> 
> 	new_value = kmemdup(data, device->report_size, GFP_ATOMIC);

[Severity: High]
This isn't a bug introduced by this patch, but does roccat_report_event()
access devices[minor] locklessly and without checking for NULL?

If a device is disconnected, roccat_disconnect() sets devices[minor] = NULL
and drops the device reference. Because sub-drivers like hid-roccat-kone fail
to call hid_hw_stop() before roccat_disconnect(), incoming USB interrupts can
still trigger roccat_report_event() concurrently.

Could this lead to a NULL pointer dereference when kmemdup() accesses
device->report_size?

> 	if (!new_value)
> 		return -ENOMEM;
> 
> 	mutex_lock(&device->readers_lock);
> 	mutex_lock(&device->cbuf_lock);

[Severity: High]
This is a pre-existing issue, but does this code acquire sleepable mutexes from
an atomic interrupt context?

roccat_report_event() is called by raw_event callbacks in HID sub-drivers,
which execute in atomic interrupt context (URB completion handlers like
usbhid_irq_in).

Can calling mutex_lock() here cause a "scheduling while atomic" panic, or
a deadlock if the lock is contended?

> drivers/hid/hid-roccat-kone.c:
> static void kone_remove_specials(struct hid_device *hdev)
> {
> 	struct usb_interface *intf = to_usb_interface(hdev->dev.parent);
> 	struct kone_device *kone;
> 
> 	if (intf->cur_altsetting->desc.bInterfaceProtocol
> 			== USB_INTERFACE_PROTOCOL_MOUSE) {
> 		kone = hid_get_drvdata(hdev);
> 		if (kone->roccat_claimed)
> 			roccat_disconnect(kone->chrdev_minor);
> 		kfree(hid_get_drvdata(hdev));
> 	}
> }
> 
> static void kone_remove(struct hid_device *hdev)
> {
> 	kone_remove_specials(hdev);
> 	hid_hw_stop(hdev);
> }

[Severity: Critical]
This isn't a bug introduced by this patch, but do HID roccat sub-drivers free
their private driver data before calling hid_hw_stop()?

During device unbinding/removal, kone_remove_specials() is called which frees
the driver data with kfree(hid_get_drvdata(hdev)). However, hid_hw_stop() is
called after this free.

Since hid_hw_stop() is responsible for stopping hardware interrupts, an incoming
USB report can trigger kone_raw_event() concurrently:

drivers/hid/hid-roccat-kone.c:kone_raw_event() {
	struct kone_device *kone = hid_get_drvdata(hdev);
	...
	if (kone == NULL)
		return 0;
	...
	if (kone->roccat_claimed)
		kone_report_to_chrdev(kone, event);
}

Can this result in a Use-After-Free memory corruption when the raw_event
callback dereferences the freed driver data?

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