Re: [PATCH v4 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: use-after-free of device->dev in roccat_device_available()
--

commit 10e7416f7d2b9142317f413c7988155b077d1914
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 the device
is registered rather than relying on a boolean exist flag. It replaces
the flag checks in the read, poll, and release paths with the new helper.

> 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]
Does this lockless check risk a use-after-free of the dev pointer in
drivers/hid/hid-roccat.c?

If a userspace process calls roccat_read() or roccat_poll() on the character
device while the physical device is simultaneously unplugged,
roccat_disconnect() can run concurrently. Since roccat_disconnect() unregisters
and drops the reference to the device, dev could be freed after the
READ_ONCE() but before device_is_registered() accesses
dev->kobj.state_in_sysfs.

> +}
> +
[ ... ]
> @@ -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]
Could this synchronously free the struct device while concurrent readers
still hold the raw pointer in roccat_disconnect() in drivers/hid/hid-roccat.c?

device_destroy() synchronously unregisters and drops the final reference to
the struct device, freeing it. Because roccat_device_available() doesn't
hold a reference or lock when checking device_is_registered(), this opens a
time-of-check to time-of-use (TOCTOU) race window.

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

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