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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Critical] Removal of the unconditional `devices[minor] = NULL` in `roccat_disconnect()` leaves a dangling pointer in the global array, allowing racing `open()` calls to access a freed `hid_device`.
- [High] Mismatched kref accounting permanently leaks `struct roccat_device` references on every open cycle.

Pre-existing issues:
- [Critical] Pre-existing logic flaw: `roccat_release()` incorrectly looks up the device via the global `devices` array instead of using `file->private_data`.
--

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

    HID: roccat: use kref to manage device instances

This patch introduces a kref to manage the lifecycle of roccat_device
instances. It replaces custom lifecycle tracking in an attempt to resolve
Use-After-Free conditions between open/release and connect/disconnect.

Link: https://syzkaller.appspot.com/bug?extid=d632e93ffcd1452bc61e

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 5deb6da8d4f74..264977e8e3010 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c

[ ... ]

> @@ -193,6 +196,7 @@ static int roccat_open(struct inode *inode, struct file *file)
>  		}
>  	}
>  
> +	kref_get(&device->ref);

[Severity: High]
Does this unconditionally acquire a reference for every open file descriptor
without a corresponding unconditional release?

In roccat_connect(), the kref is initialized to 1. Here in roccat_open(), a
reference is added for every reader.

However, roccat_release() only drops a reference if it is the last reader and
the device is already disconnected. Any open/close cycle while the device is
still connected will leak a reference.

[ ... ]

> @@ -235,7 +239,8 @@ static int roccat_release(struct inode *inode, struct file *file)
>  			hid_hw_power(device->hid, PM_HINT_NORMAL);
>  			hid_hw_close(device->hid);
>  		} else {
> -			roccat_free_device(device);
> +			if (kref_put(&device->ref, roccat_free_device))
> +				devices[minor] = NULL;
>  		}
>  	}

[Severity: Critical]
This is a pre-existing issue, but does roccat_release() look up the device
incorrectly?

roccat_release() retrieves the device from the global array:

    device = devices[minor];

If the original device was disconnected and a new device connected in the same
minor slot, this lookup grabs the new device instead of the one associated
with the closing file descriptor via reader->device. This can corrupt the
readers list and incorrectly alter the open count of the new device.

[Severity: High]
Additionally, since kref_put() is only called in the else branch (when it is
the last reader and the device no longer exists), won't this leak references
for all standard open/close operations when the device is still connected?

[ ... ]

> @@ -367,21 +373,22 @@ void roccat_disconnect(int minor)
>  	struct roccat_device *device;
>  
>  	mutex_lock(&devices_lock);
> +
>  	device = devices[minor];
> +	if (!device)
> +		goto out;
>  
>  	device->exist = 0; /* TODO exist maybe not needed */
> -
>  	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
>  
> -	devices[minor] = NULL;
> -

[Severity: Critical]
Can removing this unconditional clearance leave a dangling pointer in the
global devices array?

If the device is disconnected while held open, the open count is > 0, so the
kref_put() below is bypassed. This means devices[minor] is not cleared to
NULL.

After disconnect, the HID subsystem will eventually free the underlying
device->hid. If a user subsequently closes their file descriptor, the
device->open count reaches 0. Due to the kref leaks, the device structure is
still not freed.

If a new roccat_open() call occurs, it will find the stale pointer in
devices[minor]. Because device->open is now 0, it will execute:

    error = hid_hw_power(device->hid, PM_HINT_FULLON);

Can this cause a use-after-free on the hid_device pointer?

>  	if (device->open) {
>  		hid_hw_close(device->hid);
>  		wake_up_interruptible(&device->wait);
>  	} else {
> -		roccat_free_device(device);
> +		if (kref_put(&device->ref, roccat_free_device))
> +			devices[minor] = NULL;
>  	}

[Severity: High]
Does this also fail to drop the initial connection reference if the device is
disconnected while open?

Because the kref_put() is only in the else branch, a device that is unplugged
while open will never drop its initial reference from roccat_connect(),
causing it to permanently leak.

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