Re: [PATCH v5 1/3] 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: Use-After-Free in roccat_device_available() via device_is_registered()
--

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

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

This patch introduces roccat_device_available() to check if a device is
available (using device_is_registered()), removing the explicit 'exist'
flag from struct roccat_device.

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 5deb6da8d4f74..96aa508111a1b 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -70,6 +69,13 @@ 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)
> +{
> +	struct device *dev = READ_ONCE(device->dev);
> +
> +	return dev ? device_is_registered(dev) : false;

[Severity: High]
Are we dereferencing the cached dev pointer here without holding a reference
count?

If a thread reads the dev pointer here, gets preempted while device_destroy()
frees it, and then evaluates device_is_registered(dev), could this lead to a
Use-After-Free?

This is not fixed by subsequent patches in the series (e.g., commit
"HID: roccat: use kref to manage device instances"), which only refcount
the struct roccat_device, not the struct device.

> +}
> +
>  static void roccat_free_device(struct roccat_device *device)
>  {
>  	int i;
[ ... ]
> @@ -149,7 +155,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))

[Severity: High]
Is it possible for a lockless caller like roccat_poll() to trigger the
Use-After-Free vulnerability mentioned above by reading dev concurrently?

>  		return EPOLLERR | EPOLLHUP;
>  	return 0;
>  }
[ ... ]
> @@ -369,10 +374,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));
> -
> +	WRITE_ONCE(device->dev, NULL);

[Severity: High]
Can this sequence unregister and free the struct device memory concurrently
before the pointer is cleared, causing the Use-After-Free?

Could device_destroy() drop the final reference and free the struct device
memory immediately before WRITE_ONCE() clears the pointer?

>  	devices[minor] = NULL;
>  
>  	if (device->open) {

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