Re: [PATCH] HID: roccat: fix use-after-free and uninitialized-mutex races

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

Pre-existing issues:
- [Critical] roccat_release looks up the device by minor from the global array instead of using reader->device, causing memory leaks and cross-device UAF/list corruption after a disconnect.
- [Critical] Multiple roccat sub-drivers free their private data before calling hid_hw_stop(), creating a Use-After-Free window in raw_event handlers.
- [High] roccat_report_event() accesses devices[minor] locklessly and without NULL checks, leading to a NULL pointer dereference.
- [High] roccat_report_event() calls mutex_lock() while executing in an atomic context.
--

commit f1bc16383a0353c41082cf198a1495d5e17e528e
Author: Deepanshu Kartikey [off-list ref]

    HID: roccat: fix use-after-free and uninitialized-mutex races

This commit fixes a use-after-free in roccat_disconnect() by keeping the
whole lookup, state update, and free decision inside a single devices_lock
critical section. It also fixes an uninitialized-mutex race in
roccat_connect().

Closes: https://syzkaller.appspot.com/bug?extid=c492a9e154f81127551f

[ ... ]

[Severity: Critical]
This is a pre-existing issue, but does roccat_release() mishandle the device
lookup after this disconnect clears devices[minor]?

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

If a user holds an open file descriptor while the USB device is disconnected,
roccat_disconnect() clears devices[minor] to NULL here. When the user later
closes the file descriptor, roccat_release() returns early because
devices[minor] is NULL, which seems to leak both roccat_reader and
roccat_device.

Also, if a new device is plugged in before the old file descriptor is closed,
reusing the minor number, won't roccat_release() fetch the new device and
incorrectly delete the old reader from the new device's list, corrupting the
list and the open count?

[Severity: High]
This isn't a bug introduced by this patch, but does roccat_report_event() lack
NULL checks and synchronization when accessing the global array?

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

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

If an incoming hardware interrupt invokes this function concurrently with a
physical disconnection, could roccat_disconnect() clearing devices[minor] here
cause roccat_report_event() to dereference a NULL pointer for
device->report_size?

[Severity: High]
This is a pre-existing issue, but is it safe to use mutex_lock() inside
roccat_report_event()?

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

Since roccat_report_event() is called from the driver's .raw_event handlers
(like kone_raw_event), which execute in an atomic context (such as URB
completion softirq or hardirq), won't these mutexes sleep and lead to a
scheduling while atomic panic?

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

[ ... ]

[Severity: Critical]
This isn't a bug introduced by this patch, but does kone_remove() free
resources before stopping the hardware?

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 a hardware interrupt arrives between the kfree() in kone_remove_specials()
and hid_hw_stop(), couldn't the HID core route it to the kone_raw_event()
handler? That handler would then dereference the dangling private data pointer,
causing a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903121206.6893-1-kartikey406@gmail.com?part=1