Re: [PATCH v6 2/2] HID: roccat: use kref to manage device instances

[sashiko-bot@kernel.org]

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

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

HID: roccat: use kref to manage device instances

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.

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 92a9b9f14f591..581dedde6d66e 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
[ ... ]
> @@ -370,23 +380,25 @@ EXPORT_SYMBOL_GPL(roccat_connect);
>  void roccat_disconnect(int minor)
>  {
>  	struct roccat_device *device;
> -	struct device *dev;
>  
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
> +	if (WARN_ON(!device))
> +		goto out;
>  
> -	dev = xchg(&device->dev, NULL);
> -	device_destroy(dev->class, MKDEV(roccat_major, minor));
> +	if (!WARN_ON(!roccat_device_available(device))) {
> +		struct device *dev = xchg(&device->dev, NULL);
>  
> -	devices[minor] = NULL;
> +		device_destroy(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);
>  }

[Severity: High]
Can this concurrent disconnect path trigger a use-after-free in readers?

If CPU1 calls roccat_poll() or roccat_read(), they invoke
roccat_device_available() without holding devices_lock. That helper loads the
pointer locklessly:

drivers/hid/hid-roccat.c:roccat_device_available() {
	struct device *dev = smp_load_acquire(&device->dev);

	return dev ? device_is_registered(dev) : false;
}

If CPU1 is preempted right after smp_load_acquire(), and CPU2 runs
roccat_disconnect(), CPU2 will exchange device->dev to NULL and call
device_destroy(). This drops the final reference and frees the device memory.

When CPU1 resumes, it will pass the now-freed dev pointer to
device_is_registered(), causing a use-after-free when accessing
dev->kobj.state_in_sysfs.

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