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

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

[Deepanshu Kartikey]

roccat_disconnect() dropped devices_lock between reading device->exist
and later checking device->open to decide whether to free the device,
letting a concurrent roccat_release() free it first and turning the
later check into a use-after-free. Fix by keeping the whole lookup,
state update, and free-or-keep decision inside one devices_lock
critical section.

roccat_connect() also published the device via devices[minor] and
device_create() before initializing device->readers_lock and
device->cbuf_lock, letting a racing open() on the newly created node
take an uninitialized mutex. Fix by completing all initialization
before publishing the device.

Reported-by: syzbot+c492a9e154f81127551f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=c492a9e154f81127551f
Tested-by: syzbot+c492a9e154f81127551f@syzkaller.appspotmail.com
Signed-off-by: Deepanshu Kartikey <kartikey406@gmail.com>
---
 drivers/hid/hid-roccat.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 4f15eb951039..1d57f8a44a3d 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -316,6 +316,14 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 	if (!device)
 		return -ENOMEM;
 
+	init_waitqueue_head(&device->wait);
+	INIT_LIST_HEAD(&device->readers);
+	mutex_init(&device->readers_lock);
+	mutex_init(&device->cbuf_lock);
+	device->hid = hid;
+	device->exist = 1;
+	device->cbuf_end = 0;
+	device->report_size = report_size;
 	mutex_lock(&devices_lock);
 
 	for (minor = 0; minor < ROCCAT_MAX_DEVICES; ++minor) {
@@ -324,37 +332,28 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 		break;
 	}
 
-	if (minor < ROCCAT_MAX_DEVICES) {
-		devices[minor] = device;
-	} else {
+	if (minor >= ROCCAT_MAX_DEVICES) {
 		mutex_unlock(&devices_lock);
 		kfree(device);
 		return -EINVAL;
 	}
 
+	device->minor = minor;
+
 	device->dev = device_create(klass, &hid->dev,
 			MKDEV(roccat_major, minor), NULL,
 			"%s%s%d", "roccat", hid->driver->name, minor);
 
 	if (IS_ERR(device->dev)) {
-		devices[minor] = NULL;
 		mutex_unlock(&devices_lock);
 		temp = PTR_ERR(device->dev);
 		kfree(device);
 		return temp;
 	}
 
-	mutex_unlock(&devices_lock);
+	devices[minor] = device;
 
-	init_waitqueue_head(&device->wait);
-	INIT_LIST_HEAD(&device->readers);
-	mutex_init(&device->readers_lock);
-	mutex_init(&device->cbuf_lock);
-	device->minor = minor;
-	device->hid = hid;
-	device->exist = 1;
-	device->cbuf_end = 0;
-	device->report_size = report_size;
+	mutex_unlock(&devices_lock);
 
 	return minor;
 }
@@ -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);
 
 	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);
 
-- 
2.43.0

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

[sashiko-bot]

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 <kartikey406@gmail.com>

    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

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 4f15eb9510396..1d57f8a44a3dc 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;

[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