Re: [PATCH 1/2] HID: roccat: bound device-supplied profile index

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Closing a stale roccat character device file descriptor after device removal causes a memory leak, state corruption and Use-After-Free.
- [High] `kone_get_profile()` uses an embedded, unaligned struct field (`&kone->profiles[i]`) as a DMA buffer for inbound USB control messages, leading to cache-line sharing corruption.
--

commit cda04d8cb7b50c97cbcee76f405a2121b1b4c56d
Author: Michael Bommarito <michael.bommarito@gmail.com>

HID: roccat: bound device-supplied profile index

kone_keep_values_up_to_date() and kone_profile_activated() use an
8-bit, device-supplied profile value as an index into the 5-element
kone->profiles[] array without a range check. A malicious USB device
claiming the Roccat Kone id can send a switch-profile event (or a
startup_profile read at probe) with an out-of-range value and make the
driver read out of bounds; the result is exposed via the actual_dpi
sysfs attribute.

> diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
> index 58654cf78f0df..17495fcc8b7da 100644
> --- a/drivers/hid/hid-roccat-kone.c
> +++ b/drivers/hid/hid-roccat-kone.c

[ ... ]

> @@ -793,8 +795,10 @@ static void kone_keep_values_up_to_date(struct kone_device *kone,
>  {
>  	switch (event->event) {
>  	case kone_mouse_event_switch_profile:
> -		kone->actual_dpi = kone->profiles[event->value - 1].
> -				startup_dpi;
> +		if (event->value >= 1 &&
> +		    event->value <= ARRAY_SIZE(kone->profiles))
> +			kone->actual_dpi =
> +				kone->profiles[event->value - 1].startup_dpi;
>  		fallthrough;
>  	case kone_mouse_event_osd_profile:
>  		kone->actual_profile = event->value;

[Severity: High]
This is a pre-existing issue, but does kone_get_profile() use an embedded,
unaligned array element as a DMA buffer?

In kone_init_kone_device_struct(), we call kone_get_profile() passing a
pointer to the embedded array element:

drivers/hid/hid-roccat-kone.c:kone_init_kone_device_struct() {
    ...
    retval = kone_get_profile(usb_dev, &kone->profiles[i], i + 1);
    ...
}

Which then passes the unaligned buffer directly to usb_control_msg() for a
USB_DIR_IN transfer:

drivers/hid/hid-roccat-kone.c:kone_get_profile() {
    ...
    len = usb_control_msg(usb_dev, usb_rcvctrlpipe(usb_dev, 0),
            USB_REQ_CLEAR_FEATURE,
            USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN,
            kone_command_profile, number, buf,
            sizeof(struct kone_profile), USB_CTRL_SET_TIMEOUT);
    ...
}

Since sizeof(struct kone_profile) is 975 bytes, the array elements are not
cache-line aligned and share cache lines with other struct kone_device
members. Because hid_hw_start() is called earlier, the device can generate
interrupts and concurrent writes to these shared fields may occur.

On architectures with non-coherent DMA caches, won't the cache invalidation
required for the inbound USB DMA wipe out these concurrent CPU writes,
leading to state corruption?


[Severity: High]
This is a pre-existing issue, but can roccat_release() cause a memory leak,
list corruption, and Use-After-Free if a stale file descriptor is closed
after device removal?

When closing the file descriptor, roccat_release() retrieves the device from
the global array instead of using the local reader->device:

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

If the original device was disconnected, devices[minor] is cleared, causing
roccat_release() to return -ENODEV and permanently leak the roccat_reader
allocation and the original roccat_device.

If a new device is plugged in before the stale file descriptor is closed,
devices[minor] resolves to the new device. roccat_release() then operates on
the new device, locking its readers_lock while modifying the old device's
list:

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    list_del(&reader->node);
    ...
    if (!--device->open) {
    ...
}

Could this lead to list corruption and incorrectly decrement the new device's
open count, causing premature shutdown and a Use-After-Free of the new device
when legitimate users close their descriptors?

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