[PATCH v7 0/3] Last remaining Roccat HID fixes

[PATCH v7 0/3] Last remaining Roccat HID fixes

[Dmitry Antipov]

Since Sashiko is strongly concerned about possible races, here is
a bit more conservative approach plus one more fix for possible
roccat_poll() race against roccat_read() and roccat_report_event().

Dmitry Antipov (3):
  HID: roccat: use atomic_t for device status flag
  HID: roccat: use kref to manage device instances
  HID: roccat: avoid unlocked report buffer access in roccat_poll()

 drivers/hid/hid-roccat.c | 61 +++++++++++++++++++++++++++-------------
 1 file changed, 41 insertions(+), 20 deletions(-)

-- 
2.56.0

[PATCH v7 1/3] HID: roccat: use atomic_t for device status flag

[Dmitry Antipov]

In roccat_disconnect(), per-device 'exist' status flag is cleared
with 'devices_lock' locked but both roccat_read() and roccat_poll()
checks this flag unlocked. To avoid data races, convert the flag
to atomic_t and introduce the convenient roccat_device_available()
wrapper.

Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v7: step back to explicit flag of atomic_t type
v6: adjusted to match for-7.4/roccat branch of
    https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git,
    switch to xchg()/smp_load_acquire() and fix possible
    race due to wrong logic in roccat_disconnect()
v5: adjusted to match 7.3-rc5
v4: use READ_ONCE() and WRITE_ONCE() for device access
v3: initial version to join the series
---
 drivers/hid/hid-roccat.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index b3e69c79c4b4..c76929c0456e 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -40,7 +40,7 @@ struct roccat_device {
 	unsigned int minor;
 	int report_size;
 	int open;
-	int exist;
+	atomic_t exist;
 	wait_queue_head_t wait;
 	struct device *dev;
 	struct hid_device *hid;
@@ -70,6 +70,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 atomic_read(&device->exist);
+}
+
 static void roccat_free_device(struct roccat_device *device)
 {
 	int i;
@@ -105,7 +110,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 +154,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;
 }
@@ -226,7 +231,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 {
@@ -345,7 +350,7 @@ 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;
+	atomic_set(&device->exist, 1);
 	device->cbuf_end = 0;
 	device->report_size = report_size;
 
@@ -364,7 +369,7 @@ void roccat_disconnect(int minor)
 	mutex_lock(&devices_lock);
 	device = devices[minor];
 
-	device->exist = 0; /* TODO exist maybe not needed */
+	atomic_set(&device->exist, 0);
 
 	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
 
-- 
2.56.0

[PATCH v7 2/3] 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
Reported-by: syzbot+c492a9e154f81127551f@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e
Closes: https://syzkaller.appspot.com/bug?extid=c492a9e154f81127551f
Link: https://sashiko.dev/#/patchset/20260902094551.200587-1-dmantipov@yandex.ru?part=2
Signed-off-by: Dmitry Antipov <dmantipov@yandex.ru>
---
v7: use restored device status flag again
v6: adjusted to match for-7.4/roccat branch of
    https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git
v5: adjusted to match 7.3-rc5
v4: unchanged
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 | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index c76929c0456e..5cc523635445 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;
 	atomic_t exist;
 	wait_queue_head_t wait;
 	struct device *dev;
@@ -75,12 +76,20 @@ static bool roccat_device_available(struct roccat_device *device)
 	return atomic_read(&device->exist);
 }
 
-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);
 }
 
@@ -174,7 +183,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;
@@ -196,6 +205,7 @@ static int roccat_open(struct inode *inode, struct file *file)
 			--device->open;
 			goto exit_err_readers;
 		}
+		kref_get(&device->ref);
 	}
 
 	reader->device = device;
@@ -234,9 +244,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);
@@ -348,6 +357,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;
 	atomic_set(&device->exist, 1);
@@ -369,19 +379,19 @@ void roccat_disconnect(int minor)
 	mutex_lock(&devices_lock);
 	device = devices[minor];
 
-	atomic_set(&device->exist, 0);
-
-	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
+	if (WARN_ON(!device))
+		goto out;
 
-	devices[minor] = NULL;
+	if (!WARN_ON(!atomic_cmpxchg(&device->exist, 1, 0)))
+		device_destroy(device->dev->class, MKDEV(roccat_major, minor));
 
 	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);
@@ -396,7 +406,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.56.0

[PATCH v7 3/3] HID: roccat: avoid unlocked report buffer access in roccat_poll()

[Dmitry Antipov]

Since roccat_read() and roccat_report_event() uses per-device report
buffer holding 'cbuf_lock', fix roccat_poll() to do the same to avoid
race against them. Check for an actual device presense with recently
introduced roccat_device_available() before doing actuall poll as well.

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

diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
index 5cc523635445..0681b9cec93b 100644
--- a/drivers/hid/hid-roccat.c
+++ b/drivers/hid/hid-roccat.c
@@ -160,12 +160,18 @@ static ssize_t roccat_read(struct file *file, char __user *buffer,
 static __poll_t roccat_poll(struct file *file, poll_table *wait)
 {
 	struct roccat_reader *reader = file->private_data;
-	poll_wait(file, &reader->device->wait, wait);
-	if (reader->cbuf_start != reader->device->cbuf_end)
-		return EPOLLIN | EPOLLRDNORM;
+	bool avail;
+
 	if (!roccat_device_available(reader->device))
 		return EPOLLERR | EPOLLHUP;
-	return 0;
+
+	poll_wait(file, &reader->device->wait, wait);
+
+	mutex_lock(&reader->device->cbuf_lock);
+	avail = (reader->cbuf_start != reader->device->cbuf_end);
+	mutex_unlock(&reader->device->cbuf_lock);
+
+	return avail ? EPOLLIN | EPOLLRDNORM : 0;
 }
 
 static int roccat_open(struct inode *inode, struct file *file)
-- 
2.56.0

Re: [PATCH v7 3/3] HID: roccat: avoid unlocked report buffer access in roccat_poll()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] HID: roccat: lost wakeup race with disconnect in roccat_poll()
--

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

HID: roccat: avoid unlocked report buffer access in roccat_poll()

This commit acquires cbuf_lock when checking the per-device report buffer
indices in roccat_poll() and moves the roccat_device_available() check
before poll_wait().

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 098dd85aa036a..2782fb5773d9d 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -160,12 +160,18 @@ static ssize_t roccat_read(struct file *file, char __user *buffer,
>  static __poll_t roccat_poll(struct file *file, poll_table *wait)
>  {
>  	struct roccat_reader *reader = file->private_data;
> -	poll_wait(file, &reader->device->wait, wait);
> -	if (reader->cbuf_start != reader->device->cbuf_end)
> -		return EPOLLIN | EPOLLRDNORM;
> +	bool avail;
> +
>  	if (!roccat_device_available(reader->device))
>  		return EPOLLERR | EPOLLHUP;
> -	return 0;
> +
> +	poll_wait(file, &reader->device->wait, wait);
> +
> +	mutex_lock(&reader->device->cbuf_lock);
> +	avail = (reader->cbuf_start != reader->device->cbuf_end);
> +	mutex_unlock(&reader->device->cbuf_lock);
> +
> +	return avail ? EPOLLIN | EPOLLRDNORM : 0;
>  }
>  
>  static int roccat_open(struct inode *inode, struct file *file)

[Severity: High]
Can moving the roccat_device_available() check before poll_wait() without
re-checking it afterwards cause a lost wakeup on disconnect?

If roccat_disconnect() runs on another CPU after roccat_device_available()
returns true and before poll_wait() adds the caller to reader->device->wait:

drivers/hid/hid-roccat.c:roccat_disconnect() {
	...
	if (!WARN_ON(!atomic_cmpxchg(&device->exist, 1, 0)))
		device_destroy(device->dev->class, MKDEV(roccat_major, minor));

	if (device->open) {
		hid_hw_close(device->hid);
		wake_up_interruptible(&device->wait);
	}
	...
}

the disconnect wakeup fires before the task is on the waitqueue.
roccat_poll() then registers on reader->device->wait, sees an empty buffer
(avail == false), and returns 0, leaving poll(), select(), or epoll_wait()
sleeping indefinitely on the disconnected device.

Also, in roccat_read(), unread events in the circular buffer are drained
before disconnect is reported:

drivers/hid/hid-roccat.c:roccat_read() {
	...
	/* no data? */
	if (reader->cbuf_start == device->cbuf_end) {
		add_wait_queue(&device->wait, &wait);
		set_current_state(TASK_INTERRUPTIBLE);

		/* wait for data */
		while (reader->cbuf_start == device->cbuf_end) {
			...
			if (!roccat_device_available(device)) {
				retval = -EIO;
				break;
			}
	...
}

When roccat_device_available() is checked before reader->cbuf_start !=
reader->device->cbuf_end, doesn't roccat_poll() return EPOLLERR | EPOLLHUP
without EPOLLIN | EPOLLRDNORM even when unread reports remain in the
circular buffer?

Should poll_wait() remain at the start of roccat_poll(), followed by checking
both the circular buffer under cbuf_lock and roccat_device_available()?

([`drivers/hid/hid-roccat.c:160`](drivers/hid/hid-roccat.c#L160))

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