Re: [PATCH] HID: roccat: fully initialize device before publishing to devices[]

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] `roccat_release()` leaks memory and corrupts state by looking up the device from the global array instead of the reader struct when handling hot-unplug.
- [High] `roccat_report_event()` calls sleepable `mutex_lock()` from within a hardirq/atomic context.
- [High] A race between `roccat_disconnect()` and `roccat_release()` causes a Use-After-Free of the `roccat_device` structure.
--

commit 46468883b2be9805a5a149d03c123b3d8fe1868c
Author: Ivy Lopez <skunkolee@gmail.com>

    HID: roccat: fully initialize device before publishing to devices[]
    
    The patch moves the initialization of a roccat device's private state ahead
    of the point where it is inserted into the global devices array. This ensures
    the object is fully constructed before it becomes visible to concurrent
    readers.
    
    Reported-by: syzbot+9f7405999979761b6cfc@syzkaller.appspotmail.com
    Signed-off-by: Ivy Lopez <skunkolee@gmail.com>

> diff --git a/drivers/hid/hid-roccat.c b/drivers/hid/hid-roccat.c
> index 4f15eb9510396..2d8b0da6fbb21 100644
> --- a/drivers/hid/hid-roccat.c
> +++ b/drivers/hid/hid-roccat.c
> @@ -316,26 +316,32 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
>  	if (!device)
>  		return -ENOMEM;
>  
> -	mutex_lock(&devices_lock);
> +	init_waitqueue_head(&device->wait);
> +	INIT_LIST_HEAD(&device->readers);
> +	mutex_init(&device->readers_lock);
> +	mutex_init(&device->cbuf_lock);
> +	device->hid = hid;
> +	device->exist = 1;
> +	device->cbuf_end = 0;
> +	device->report_size = report_size;
>  
> +	mutex_lock(&devices_lock);
>  	for (minor = 0; minor < ROCCAT_MAX_DEVICES; ++minor) {
>  		if (devices[minor])
>  			continue;
>  		break;
>  	}
> -
> -	if (minor < ROCCAT_MAX_DEVICES) {
> -		devices[minor] = device;
> -	} else {
> +	if (minor >= ROCCAT_MAX_DEVICES) {
>  		mutex_unlock(&devices_lock);
>  		kfree(device);
>  		return -EINVAL;
>  	}
> +	device->minor = minor;
> +	devices[minor] = device;
>  
>  	device->dev = device_create(klass, &hid->dev,
>  			MKDEV(roccat_major, minor), NULL,
>  			"%s%s%d", "roccat", hid->driver->name, minor);
> -
>  	if (IS_ERR(device->dev)) {
>  		devices[minor] = NULL;
>  		mutex_unlock(&devices_lock);
> @@ -343,19 +349,8 @@ int roccat_connect(const struct class *klass, struct hid_device *hid, int report
>  		kfree(device);
>  		return temp;
>  	}
> -
>  	mutex_unlock(&devices_lock);
>  
> -	init_waitqueue_head(&device->wait);
> -	INIT_LIST_HEAD(&device->readers);
> -	mutex_init(&device->readers_lock);
> -	mutex_init(&device->cbuf_lock);
> -	device->minor = minor;
> -	device->hid = hid;
> -	device->exist = 1;
> -	device->cbuf_end = 0;
> -	device->report_size = report_size;
> -
>  	return minor;
>  }
>  EXPORT_SYMBOL_GPL(roccat_connect);

[Severity: Critical]
This is a pre-existing issue, but does roccat_release() leak memory and
corrupt state by looking up the device from the global array instead of the
reader struct when handling hot-unplug?

If the device is unplugged, roccat_disconnect() clears the global minor
pointer:

drivers/hid/hid-roccat.c:roccat_disconnect() {
    ...
	mutex_lock(&devices_lock);
	devices[minor] = NULL;
	mutex_unlock(&devices_lock);
    ...
}

If userspace later closes the file descriptor, roccat_release() retrieves the
device from the global array rather than using the safe reader->device pointer:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
	mutex_lock(&devices_lock);

	device = devices[minor];
	if (!device) {
		mutex_unlock(&devices_lock);
		pr_emerg("roccat device with minor %d doesn't exist\n", minor);
		return -ENODEV;
	}
    ...
}

Does this cause an early return that permanently leaks the device and reader
structs?

If a new Roccat device is plugged in before the file descriptor is closed,
could it assume the old minor slot, causing roccat_release() to operate on the
new device, prematurely decrementing its open count and potentially powering it
down while the old device leaks?

[Severity: High]
This isn't a bug introduced by this patch, but does roccat_report_event() call
sleepable mutex_lock() from within a hardirq/atomic context?

The HID core triggers the raw_event callback (e.g., kone_raw_event) from an
atomic context when the low-level transport driver completes an URB. This
callback invokes roccat_report_event(), which acquires sleepable mutexes:

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
	new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
	if (!new_value)
		return -ENOMEM;

	mutex_lock(&device->readers_lock);
	mutex_lock(&device->cbuf_lock);
    ...
}

Can this lead to a scheduling while atomic panic or system deadlock upon
receiving device events?

[Severity: High]
This is a pre-existing issue, but does a race between roccat_disconnect() and
roccat_release() cause a Use-After-Free of the roccat_device structure?

When roccat_disconnect() runs, it releases devices_lock before calling
device_destroy():

drivers/hid/hid-roccat.c:roccat_disconnect() {
    ...
	mutex_lock(&devices_lock);
	device = devices[minor];
	mutex_unlock(&devices_lock);

	device->exist = 0;

	device_destroy(device->dev->class, MKDEV(roccat_major, minor));
    ...
}

If this is preempted after setting device->exist = 0, a concurrent
roccat_release() can lock devices_lock, find devices[minor] is still non-NULL,
decrement device->open to 0, and free the device because exist is 0:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
	if (!--device->open) {
		/* removing last reader */
		if (device->exist) {
            ...
		} else {
			roccat_free_device(device);
		}
	}
    ...
}

When roccat_disconnect() resumes, is it possible that it calls device_destroy()
using the freed device->dev pointer, causing a Use-After-Free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907224426.159104-1-skunkolee@gmail.com?part=1