Re: [PATCH 1/6] HID: roccat-koneplus: reject short button reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Use-After-Free during device teardown due to incorrect removal ordering.
- [High] Incomplete Fix: Out-of-bounds read via short USB reports in the savu driver.
- [High] Memory leak of `struct roccat_reader` and `struct roccat_device` on hot-unplug.
- [High] Sleeping in atomic context due to mutex_lock in the raw_event path.
- [High] Lockless Access and Data Race on the `devices` array causing NULL pointer dereference or UAF.
- [High] Kernel information leak via sysfs attributes due to uninitialized memory copying and ignored errors.
- [Low] Unsynchronized access to `actual_profile` between process and interrupt context.
--

commit d5a3862120a746721ced4fff1b161538d9d1d57c
Author: Yousef Alhouseen <alhouseenyousef@gmail.com>

HID: roccat-koneplus: reject short button reports

The Koneplus raw-event path casts button reports to an eight-byte
structure before updating profile state and forwarding an event. A
malformed USB device can identify a shorter report as a button report
and trigger out-of-bounds reads.

Require the complete button report before either consumer sees it.

Fixes: 47dbdbffe15b ("HID: roccat: Add support for Roccat Kone[+] v2")
Cc: stable@vger.kernel.org
Signed-off-by: Yousef Alhouseen <alhouseenyousef@gmail.com>

> diff --git a/drivers/hid/hid-roccat-koneplus.c b/drivers/hid/hid-roccat-koneplus.c
> index f80a60539a967..e0f35251e81a9 100644
> --- a/drivers/hid/hid-roccat-koneplus.c
> +++ b/drivers/hid/hid-roccat-koneplus.c
> @@ -523,6 +523,10 @@ static int koneplus_raw_event(struct hid_device *hdev,
>  	if (koneplus == NULL)
>  		return 0;
>  
> +	if (data[0] == KONEPLUS_MOUSE_REPORT_NUMBER_BUTTON &&
> +	    size < sizeof(struct koneplus_mouse_report_button))
> +		return 0;
> +

[Severity: High]
This is a pre-existing issue, but does the savu driver also need a similar
check?

It looks like the savu driver lacks length validation. savu_raw_event()
forwards reports to savu_report_to_chrdev(), which casts the buffer:

drivers/hid/hid-roccat-savu.c:savu_report_to_chrdev() {
    ...
    roccat_report.type = special_report->type;
    roccat_report.data[0] = special_report->data[0];
    roccat_report.data[1] = special_report->data[1];
    ...
}

If a malicious USB device sends a 1-byte report to the savu driver, could
these accesses read out-of-bounds of the heap buffer and leak information
to userspace via the character device?

>  	koneplus_keep_values_up_to_date(koneplus, data);

[Severity: Low]
This isn't a bug introduced by this patch, but is it safe to update the
profile state here without synchronization?

koneplus->actual_profile is updated from sysfs in process context under
koneplus_lock, but it is also updated here in interrupt context via
koneplus_keep_values_up_to_date() without acquiring any locks:

drivers/hid/hid-roccat-koneplus.c:koneplus_keep_values_up_to_date() {
    ...
    koneplus_profile_activated(koneplus, button_report->data1 - 1);
    ...
}

Could this data race cause stale profile values to be read if sysfs
attributes are accessed concurrently with profile-changing USB events?

>  
>  	if (koneplus->roccat_claimed)

[Severity: High]
This is a pre-existing issue, but can this path sleep in atomic context?

koneplus_raw_event() runs in atomic interrupt context from the USB URB
completion handler. If roccat_claimed is true, it calls
koneplus_report_to_chrdev(), which eventually calls roccat_report_event():

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    mutex_lock(&device->readers_lock);
    mutex_lock(&device->cbuf_lock);
    ...
}

Would acquiring these mutexes inside the raw_event path trigger a
scheduling-while-atomic bug upon receiving valid input events?

[Severity: High]
This is another pre-existing issue, but does roccat_report_event() have a
data race with device removal?

drivers/hid/hid-roccat.c:roccat_report_event() {
    ...
    device = devices[minor];
    
    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);
    ...
}

If roccat_disconnect() runs concurrently, takes devices_lock, and sets
devices[minor] = NULL, could roccat_report_event() fetch a NULL or freed
device pointer locklessly, leading to a crash or use-after-free when
accessing device->report_size?

I also noticed a few other pre-existing issues in the ROCCAT subsystem
that might be worth addressing:

[Severity: Critical]
This is a pre-existing issue, but could there be a use-after-free during
device teardown?

In koneplus_remove(), the driver state is freed before the hardware is
stopped:

drivers/hid/hid-roccat-koneplus.c:koneplus_remove() {
    ...
    koneplus_remove_specials(hdev);
    hid_hw_stop(hdev);
    ...
}

If a USB event arrives exactly during this window before hid_hw_stop()
halts the in-flight USB URBs, koneplus_raw_event() will fetch the
freed driver data:

drivers/hid/hid-roccat-koneplus.c:koneplus_raw_event() {
    struct koneplus_device *koneplus = hid_get_drvdata(hdev);
    ...
    koneplus_keep_values_up_to_date(koneplus, data);
    ...
}

Would this dereference the freed koneplus pointer?

[Severity: High]
This is a pre-existing issue, but does hot-unplugging the device while it
is held open by userspace cause a memory leak?

When a ROCCAT device is unplugged, roccat_disconnect() clears the array slot:

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

If a user-space process holds the char device open, the memory isn't freed
immediately. When the process later closes the file descriptor,
roccat_release() checks the array:

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;
    }
    ...
}

Since the device was set to NULL during disconnect, it returns early and
bypasses freeing the reader and device structures. Are these structures
leaked permanently?

[Severity: High]
This is a pre-existing issue, but can sysfs attributes leak uninitialized
heap memory to userspace?

In roccat_common2_receive(), a buffer is allocated via kmalloc() without
GFP_ZERO. If usb_control_msg() fails, it still executes memcpy():

drivers/hid/hid-roccat-common.c:roccat_common2_receive() {
    ...
    len = usb_control_msg(...);
    
    memcpy(data, buf, size);
    ...
}

Sysfs show handlers ignore the return value and emit the potentially
uninitialized data:

drivers/hid/hid-roccat-koneplus.c:koneplus_sysfs_show_firmware_version() {
    ...
    roccat_common2_receive(usb_dev, KONEPLUS_COMMAND_INFO,
                    &info, KONEPLUS_SIZE_INFO);
    ...
    return sysfs_emit(buf, "%d\n", info.firmware_version);
}

If the USB transfer fails, does this expose uninitialized memory to any
local user able to read the sysfs files?

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