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

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

[Dmitry Antipov]

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

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/hid/hid-roccat.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 4f15eb951039..5deb6da8d4f7 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -344,8 +344,6 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 		return temp;
 	}
 
-	mutex_unlock(&devices_lock);
-
 	init_waitqueue_head(&device->wait);
 	INIT_LIST_HEAD(&device->readers);
 	mutex_init(&device->readers_lock);
@@ -356,6 +354,7 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 	device->cbuf_end = 0;
 	device->report_size = report_size;
 
+	mutex_unlock(&devices_lock);
 	return minor;
 }
 EXPORT_SYMBOL_GPL(roccat_connect);
@@ -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.55.0

[PATCH 2/2] HID: roccat: use kref to manage device instances

[Dmitry Antipov]

Use kref to manage 'struct roccat_device' instances and fix
UaF-triggering race between roccat_open()/roccat_release()
and roccat_connect()/roccat_disconnect() pairs.

Reported-by: syzbot+d632e93ffcd1452bc61e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
 drivers/hid/hid-roccat.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 5deb6da8d4f7..264977e8e301 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -41,6 +41,7 @@ struct roccat_device {
 	int report_size;
 	int open;
 	int exist;
+	struct kref ref;
 	wait_queue_head_t wait;
 	struct device *dev;
 	struct hid_device *hid;
@@ -70,10 +71,12 @@ 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)
+static void roccat_free_device(struct kref *ref)
 {
+	struct roccat_device *device;
 	int i;
 
+	device = container_of(ref, struct roccat_device, ref);
 	for (i = 0; i < ROCCAT_CBUF_SIZE; i++)
 		kfree(device->cbuf[i].value);
 	kfree(device);
@@ -193,6 +196,7 @@ static int roccat_open(struct inode *inode, struct file *file)
 		}
 	}
 
+	kref_get(&device->ref);
 	reader->device = device;
 	/* new reader doesn't get old events */
 	reader->cbuf_start = device->cbuf_end;
@@ -235,7 +239,8 @@ static int roccat_release(struct inode *inode, struct file *file)
 			hid_hw_power(device->hid, PM_HINT_NORMAL);
 			hid_hw_close(device->hid);
 		} else {
-			roccat_free_device(device);
+			if (kref_put(&device->ref, roccat_free_device))
+				devices[minor] = NULL;
 		}
 	}
 
@@ -348,6 +353,7 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 	INIT_LIST_HEAD(&device->readers);
 	mutex_init(&device->readers_lock);
 	mutex_init(&device->cbuf_lock);
+	kref_init(&device->ref);
 	device->minor = minor;
 	device->hid = hid;
 	device->exist = 1;
@@ -367,21 +373,22 @@ void roccat_disconnect(int minor)
 	struct roccat_device *device;
 
 	mutex_lock(&devices_lock);
+
 	device = devices[minor];
+	if (!device)
+		goto out;
 
 	device->exist = 0; /* TODO exist maybe not needed */
-
 	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
 
-	devices[minor] = NULL;
-
 	if (device->open) {
 		hid_hw_close(device->hid);
 		wake_up_interruptible(&device->wait);
 	} else {
-		roccat_free_device(device);
+		if (kref_put(&device->ref, roccat_free_device))
+			devices[minor] = NULL;
 	}
-
+out:
 	mutex_unlock(&devices_lock);
 }
 EXPORT_SYMBOL_GPL(roccat_disconnect);
-- 
2.55.0

Re: [PATCH 2/2] HID: roccat: use kref to manage device instances

[sashiko-bot]

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

New issues:
- [Critical] Removal of the unconditional `devices[minor] = NULL` in `roccat_disconnect()` leaves a dangling pointer in the global array, allowing racing `open()` calls to access a freed `hid_device`.
- [High] Mismatched kref accounting permanently leaks `struct roccat_device` references on every open cycle.

Pre-existing issues:
- [Critical] Pre-existing logic flaw: `roccat_release()` incorrectly looks up the device via the global `devices` array instead of using `file->private_data`.
--

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

    HID: roccat: use kref to manage device instances

This patch introduces a kref to manage the lifecycle of roccat_device
instances. It replaces custom lifecycle tracking in an attempt to resolve
Use-After-Free conditions between open/release and connect/disconnect.

Link: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 5deb6da8d4f74..264977e8e3010 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c

[ ... ]

> @@ -193,6 +196,7 @@ static int roccat_open(struct inode *inode, struct file *file)
>  		}
>  	}
>  
> +	kref_get(&device->ref);

[Severity: High]
Does this unconditionally acquire a reference for every open file descriptor
without a corresponding unconditional release?

In roccat_connect(), the kref is initialized to 1. Here in roccat_open(), a
reference is added for every reader.

However, roccat_release() only drops a reference if it is the last reader and
the device is already disconnected. Any open/close cycle while the device is
still connected will leak a reference.

[ ... ]

> @@ -235,7 +239,8 @@ static int roccat_release(struct inode *inode, struct file *file)
>  			hid_hw_power(device->hid, PM_HINT_NORMAL);
>  			hid_hw_close(device->hid);
>  		} else {
> -			roccat_free_device(device);
> +			if (kref_put(&device->ref, roccat_free_device))
> +				devices[minor] = NULL;
>  		}
>  	}

[Severity: Critical]
This is a pre-existing issue, but does roccat_release() look up the device
incorrectly?

roccat_release() retrieves the device from the global array:

    device = devices[minor];

If the original device was disconnected and a new device connected in the same
minor slot, this lookup grabs the new device instead of the one associated
with the closing file descriptor via reader->device. This can corrupt the
readers list and incorrectly alter the open count of the new device.

[Severity: High]
Additionally, since kref_put() is only called in the else branch (when it is
the last reader and the device no longer exists), won't this leak references
for all standard open/close operations when the device is still connected?

[ ... ]

> @@ -367,21 +373,22 @@ void roccat_disconnect(int minor)
>  	struct roccat_device *device;
>  
>  	mutex_lock(&devices_lock);
> +
>  	device = devices[minor];
> +	if (!device)
> +		goto out;
>  
>  	device->exist = 0; /* TODO exist maybe not needed */
> -
>  	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
>  
> -	devices[minor] = NULL;
> -

[Severity: Critical]
Can removing this unconditional clearance leave a dangling pointer in the
global devices array?

If the device is disconnected while held open, the open count is > 0, so the
kref_put() below is bypassed. This means devices[minor] is not cleared to
NULL.

After disconnect, the HID subsystem will eventually free the underlying
device->hid. If a user subsequently closes their file descriptor, the
device->open count reaches 0. Due to the kref leaks, the device structure is
still not freed.

If a new roccat_open() call occurs, it will find the stale pointer in
devices[minor]. Because device->open is now 0, it will execute:

    error = hid_hw_power(device->hid, PM_HINT_FULLON);

Can this cause a use-after-free on the hid_device pointer?

>  	if (device->open) {
>  		hid_hw_close(device->hid);
>  		wake_up_interruptible(&device->wait);
>  	} else {
> -		roccat_free_device(device);
> +		if (kref_put(&device->ref, roccat_free_device))
> +			devices[minor] = NULL;
>  	}

[Severity: High]
Does this also fail to drop the initial connection reference if the device is
disconnected while open?

Because the kref_put() is only in the else branch, a device that is unplugged
while open will never drop its initial reference from roccat_connect(),
causing it to permanently leak.

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

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

[sashiko-bot]

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