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

[sashiko-bot@kernel.org]

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