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

[PATCH v3 1/4] 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>
---
v3: likewise
v2: unchanged since v1
---
 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 v3 2/4] HID: roccat: use device_is_registered() to check whether device is available

[Dmitry Antipov]

Introduce roccat_device_available() to check whether device
is actually available (i.e. not disconnected), thus removing
explicit 'exist' flag from 'struct roccat_device'.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: initial version to join the series
---
 drivers/hid/hid-roccat.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 5deb6da8d4f7..454b201c0524 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -40,7 +40,6 @@ struct roccat_device {
 	unsigned int minor;
 	int report_size;
 	int open;
-	int exist;
 	wait_queue_head_t wait;
 	struct device *dev;
 	struct hid_device *hid;
@@ -70,6 +69,11 @@ static struct roccat_device *devices[ROCCAT_MAX_DEVICES];
 /* protects modifications of devices array */
 static DEFINE_MUTEX(devices_lock);
 
+static bool roccat_device_available(struct roccat_device *device)
+{
+	return device->dev ? device_is_registered(device->dev) : false;
+}
+
 static void roccat_free_device(struct roccat_device *device)
 {
 	int i;
@@ -105,7 +109,7 @@ static ssize_t roccat_read(struct file *file, char __user *buffer,
 				retval = -ERESTARTSYS;
 				break;
 			}
-			if (!device->exist) {
+			if (!roccat_device_available(device)) {
 				retval = -EIO;
 				break;
 			}
@@ -149,7 +153,7 @@ static __poll_t roccat_poll(struct file *file, poll_table *wait)
 	poll_wait(file, &reader->device->wait, wait);
 	if (reader->cbuf_start != reader->device->cbuf_end)
 		return EPOLLIN | EPOLLRDNORM;
-	if (!reader->device->exist)
+	if (!roccat_device_available(reader->device))
 		return EPOLLERR | EPOLLHUP;
 	return 0;
 }
@@ -231,7 +235,7 @@ static int roccat_release(struct inode *inode, struct file *file)
 
 	if (!--device->open) {
 		/* removing last reader */
-		if (device->exist) {
+		if (roccat_device_available(device)) {
 			hid_hw_power(device->hid, PM_HINT_NORMAL);
 			hid_hw_close(device->hid);
 		} else {
@@ -350,7 +354,6 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
 	mutex_init(&device->cbuf_lock);
 	device->minor = minor;
 	device->hid = hid;
-	device->exist = 1;
 	device->cbuf_end = 0;
 	device->report_size = report_size;
 
@@ -369,10 +372,8 @@ void roccat_disconnect(int minor)
 	mutex_lock(&devices_lock);
 	device = devices[minor];
 
-	device->exist = 0; /* TODO exist maybe not needed */
-
 	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
-
+	device->dev = NULL;
 	devices[minor] = NULL;
 
 	if (device->open) {
-- 
2.55.0

[PATCH v3 3/4] HID: roccat: fix device access in roccat_release()

[Dmitry Antipov]

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.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://sashiko.dev/#/patchset/20260902094551.200587-1-dmantipov@yandex.ru?part=2
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: unchanged since v2
v2: initial version to join the series
---
 drivers/hid/hid-roccat.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 454b201c0524..1e1a76c453bd 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -215,24 +215,19 @@ static int roccat_open(struct inode *inode, struct file *file)
 
 static int roccat_release(struct inode *inode, struct file *file)
 {
-	unsigned int minor = iminor(inode);
 	struct roccat_reader *reader = file->private_data;
-	struct roccat_device *device;
-
-	mutex_lock(&devices_lock);
+	struct roccat_device *device = reader->device;
 
-	device = devices[minor];
-	if (!device) {
-		mutex_unlock(&devices_lock);
-		pr_emerg("roccat device with minor %d doesn't exist\n", minor);
+	if (WARN_ON(!device))
 		return -ENODEV;
-	}
 
 	mutex_lock(&device->readers_lock);
 	list_del(&reader->node);
 	mutex_unlock(&device->readers_lock);
 	kfree(reader);
 
+	mutex_lock(&devices_lock);
+
 	if (!--device->open) {
 		/* removing last reader */
 		if (roccat_device_available(device)) {
-- 
2.55.0

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

[Dmitry Antipov]

Use kref to manage 'struct roccat_device' instances and fix both memory
leaks and UaF-triggering races between roccat_open()/roccat_release()
and roccat_connect()/roccat_disconnect() pairs. To avoid the scenario
when opened device is disconnected and its slot indexed by minor number
is reused by another device, release the slot in roccat_free_device()
rather than in roccat_disconnect(). This way, there is a time frame when
disconnected device may still occupy the slot; to reject such a device,
add extra roccat_device_available() checks to roccat_open() and
roccat_ioctl(). Add extra WARN_ON() device check to roccat_disconnect()
and a few debugging quirks to roccat_free_device() as well.

Reported-by: syzbot+d632e93ffcd1452bc61e@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e
Link: https://sashiko.dev/#/patchset/20260902094551.200587-1-dmantipov@yandex.ru?part=2
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v3: release device slot in roccat_free_device(), add required checks
    to roccat_open() and roccat_ioctl(), few more debugging quirks
v2: unconditionally get/put device reference during
    first open and last close, respectively (Sashiko)
---
 drivers/hid/hid-roccat.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 1e1a76c453bd..bdf885750929 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -40,6 +40,7 @@ struct roccat_device {
 	unsigned int minor;
 	int report_size;
 	int open;
+	struct kref ref;
 	wait_queue_head_t wait;
 	struct device *dev;
 	struct hid_device *hid;
@@ -74,12 +75,20 @@ static bool roccat_device_available(struct roccat_device *device)
 	return device->dev ? device_is_registered(device->dev) : false;
 }
 
-static void roccat_free_device(struct roccat_device *device)
+static void roccat_free_device(struct kref *ref)
 {
+	struct roccat_device *device;
 	int i;
 
+	WARN_ON(!mutex_is_locked(&devices_lock));
+
+	device = container_of(ref, struct roccat_device, ref);
 	for (i = 0; i < ROCCAT_CBUF_SIZE; i++)
 		kfree(device->cbuf[i].value);
+
+	devices[device->minor] = NULL;
+	mutex_destroy(&device->readers_lock);
+	mutex_destroy(&device->cbuf_lock);
 	kfree(device);
 }
 
@@ -173,7 +182,7 @@ static int roccat_open(struct inode *inode, struct file *file)
 
 	device = devices[minor];
 
-	if (!device) {
+	if (!device || !roccat_device_available(device)) {
 		pr_emerg("roccat device with minor %d doesn't exist\n", minor);
 		error = -ENODEV;
 		goto exit_err_devices;
@@ -195,6 +204,7 @@ static int roccat_open(struct inode *inode, struct file *file)
 			--device->open;
 			goto exit_err_readers;
 		}
+		kref_get(&device->ref);
 	}
 
 	reader->device = device;
@@ -233,9 +243,8 @@ static int roccat_release(struct inode *inode, struct file *file)
 		if (roccat_device_available(device)) {
 			hid_hw_power(device->hid, PM_HINT_NORMAL);
 			hid_hw_close(device->hid);
-		} else {
-			roccat_free_device(device);
 		}
+		kref_put(&device->ref, roccat_free_device);
 	}
 
 	mutex_unlock(&devices_lock);
@@ -347,6 +356,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->cbuf_end = 0;
@@ -366,18 +376,20 @@ void roccat_disconnect(int minor)
 
 	mutex_lock(&devices_lock);
 	device = devices[minor];
+	if (WARN_ON(!device))
+		goto out;
 
-	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
-	device->dev = NULL;
-	devices[minor] = NULL;
+	if (!WARN_ON(!roccat_device_available(device))) {
+		device_destroy(device->dev->class, MKDEV(roccat_major, minor));
+		device->dev = NULL;
+	}
 
 	if (device->open) {
 		hid_hw_close(device->hid);
 		wake_up_interruptible(&device->wait);
-	} else {
-		roccat_free_device(device);
 	}
-
+	kref_put(&device->ref, roccat_free_device);
+out:
 	mutex_unlock(&devices_lock);
 }
 EXPORT_SYMBOL_GPL(roccat_disconnect);
@@ -392,7 +404,7 @@ static long roccat_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	mutex_lock(&devices_lock);
 
 	device = devices[minor];
-	if (!device) {
+	if (!device || !roccat_device_available(device)) {
 		retval = -ENODEV;
 		goto out;
 	}
-- 
2.55.0

Re: [PATCH v3 2/4] HID: roccat: use device_is_registered() to check whether device is available

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: roccat: UAF/NULL deref in roccat_device_available() during disconnect
--

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

HID: roccat: use device_is_registered() to check whether device is available

This commit introduces roccat_device_available() to check whether a device
is actually available by calling device_is_registered(device->dev), replacing
the explicit 'exist' flag from 'struct roccat_device'.

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 5deb6da8d4f74..454b201c0524b 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -70,6 +69,11 @@ static struct roccat_device *devices[ROCCAT_MAX_DEVICES];
>  /* protects modifications of devices array */
>  static DEFINE_MUTEX(devices_lock);
>  
> +static bool roccat_device_available(struct roccat_device *device)
> +{
> +	return device->dev ? device_is_registered(device->dev) : false;
> +}
> +

[Severity: High]
Does this introduce a Use-After-Free or NULL pointer dereference race?

When roccat_device_available() is called, device->dev is evaluated. If a
concurrent thread calls roccat_disconnect() and frees the device structure via
device_destroy(), device_is_registered(device->dev) may access freed memory.
Also, a compiler double-fetch could result in a direct NULL pointer dereference.

[ ... ]
> @@ -105,7 +109,7 @@ static ssize_t roccat_read(struct file *file, char __user *buffer,
>  				retval = -ERESTARTSYS;
>  				break;
>  			}
> -			if (!device->exist) {
> +			if (!roccat_device_available(device)) {
>  				retval = -EIO;
>  				break;
>  			}

[Severity: High]
Can this race occur here?

roccat_read() calls roccat_device_available() without holding devices_lock.
This means the device's availability is evaluated locklessly while
roccat_disconnect() might be tearing down the device.

[ ... ]
> @@ -149,7 +153,7 @@ static __poll_t roccat_poll(struct file *file, poll_table *wait)
>  	poll_wait(file, &reader->device->wait, wait);
>  	if (reader->cbuf_start != reader->device->cbuf_end)
>  		return EPOLLIN | EPOLLRDNORM;
> -	if (!reader->device->exist)
> +	if (!roccat_device_available(reader->device))
>  		return EPOLLERR | EPOLLHUP;
>  	return 0;
>  }

[Severity: High]
Similarly, roccat_poll() calls roccat_device_available() without holding
devices_lock, exposing it to the same race condition as roccat_read().

[ ... ]
> @@ -369,10 +372,8 @@ void roccat_disconnect(int minor)
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
>  
> -	device->exist = 0; /* TODO exist maybe not needed */
> -
>  	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
> -
> +	device->dev = NULL;
>  	devices[minor] = NULL;
>  
>  	if (device->open) {

[Severity: High]
Could this teardown path trigger the race?

roccat_disconnect() holds devices_lock, destroys and drops the reference to
the device with device_destroy(), and then nullifies device->dev.
Since roccat_read() and roccat_poll() don't hold devices_lock, they can
dereference device->dev concurrently just before or as it is being freed.

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] hid: roccat: UaF in roccat_device_available() due to unlocked device->dev access
--

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

HID: roccat: use kref to manage device instances

This commit introduces a kref-based lifecycle for roccat_device instances
to resolve memory leaks and use-after-free issues between open/release
and connect/disconnect pairs. The device slot is now released in
roccat_free_device() instead of roccat_disconnect().

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 1e1a76c453bd2..bdf8857509294 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -366,18 +376,20 @@ void roccat_disconnect(int minor)
>  
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
> +	if (WARN_ON(!device))
> +		goto out;
>  
> -	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
> -	device->dev = NULL;
> -	devices[minor] = NULL;
> +	if (!WARN_ON(!roccat_device_available(device))) {
> +		device_destroy(device->dev->class, MKDEV(roccat_major, minor));
> +		device->dev = NULL;
> +	}

[Severity: High]
Can an unlocked reader racing with this code trigger a use-after-free?

If a userspace process calls roccat_read() or roccat_poll() on the device
node concurrently with roccat_disconnect(), they will invoke
roccat_device_available().

Since those read and poll paths are unlocked, a thread could see
device->dev as non-NULL in roccat_device_available():

drivers/hid/hid-roccat.c:roccat_device_available() {
    return device->dev ? device_is_registered(device->dev) : false;
}

If it is preempted right after the ternary check but before calling
device_is_registered(), and this roccat_disconnect() code then runs,
destroying device->dev and setting it to NULL, the reader thread will
resume and dereference the freed pointer.

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