Re: [PATCH v3 2/4] HID: roccat: use device_is_registered() to check whether device is available
From: sashiko-bot@kernel.org
Date: 2026-09-16 08:09:03
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 [off-list ref] 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'.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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. [ ... ]
quoted hunk ↗ jump to hunk
@@ -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(). [ ... ]
quoted hunk ↗ jump to hunk
@@ -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