Re: [PATCH v6 1/2] 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 2 potential issue(s) to consider:
- [High] hid: Use-After-Free of struct device in roccat_device_available()
- [High] hid: roccat: UAF in roccat_device_available() during device removal
--

commit 55c9b90288fdb8c7ef6b2e3456eeca1cc08711d4
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 if a device is
registered. It removes the explicit 'exist' flag from struct roccat_device.

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 5deb6da8d4f74..92a9b9f14f591 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 = smp_load_acquire(&device->dev);
> +
> +	return dev ? device_is_registered(dev) : false;

[Severity: High]
Does this create a Use-After-Free risk when evaluated concurrently with device
removal?

If roccat_read() or roccat_poll() fetches the 'dev' pointer via
smp_load_acquire() but is then preempted before calling
device_is_registered():

Thread 1 (roccat_read/roccat_poll)
roccat_device_available()
    struct device *dev = smp_load_acquire(&device->dev);
    <preempted>

Thread 2 (roccat_disconnect)
    dev = xchg(&device->dev, NULL);
    device_destroy(dev->class, MKDEV(roccat_major, minor));

Since device_destroy() synchronously drops the reference and can free the
underlying struct device memory, could Thread 1 then resume and pass the freed
'dev' pointer to device_is_registered(), dereferencing freed memory?

[ ... ]

> @@ -365,13 +370,13 @@ EXPORT_SYMBOL_GPL(roccat_connect);
>  void roccat_disconnect(int minor)
>  {
>  	struct roccat_device *device;
> +	struct device *dev;
>  
>  	mutex_lock(&devices_lock);
>  	device = devices[minor];
>  
> -	device->exist = 0; /* TODO exist maybe not needed */
> -
> -	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
> +	dev = xchg(&device->dev, NULL);
> +	device_destroy(dev->class, MKDEV(roccat_major, minor));

[Severity: High]
Since roccat_device_available() can be called from read and poll handlers
without holding devices_lock, does swapping the pointer and immediately
destroying the device leave concurrent readers unprotected?

>  
>  	devices[minor] = NULL;
>

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