Re: [PATCH 5/6] HID: roccat-konepure: reject short button reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch series fixes out-of-bounds reads in several Roccat drivers by adding report size validation, but misses identical vulnerabilities in `hid-roccat-savu.c`.
- [High] The `roccat_report_event()` function illegally acquires sleeping mutex locks while executing in atomic/softirq context, leading to scheduling-while-atomic bugs.
- [High] Leaking `struct roccat_device` and `struct roccat_reader` in `roccat_release()` after device disconnect.
- [High] Leaking dynamically allocated report buffers in the ring buffer when destroying `roccat_device`.
- [High] Missing size checks in `corsair_void_raw_event()` allow out-of-bounds memory reads.
--

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

HID: roccat-konepure: reject short button reports
    
konepure_raw_event() forwards button reports directly to
roccat_report_event(), which copies the fixed eight-byte report size
registered by this driver. A malformed USB device can send a shorter
report and make that copy read beyond the input buffer.

> diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c
> index 7f753dfc2a108..d17dee18ac2b0 100644
> --- a/drivers/hid/hid-roccat-konepure.c
> +++ b/drivers/hid/hid-roccat-konepure.c
> @@ -181,7 +181,8 @@ static int konepure_raw_event(struct hid_device *hdev,
>  			!= USB_INTERFACE_PROTOCOL_MOUSE)
>  		return 0;
>  
> -	if (data[0] != KONEPURE_MOUSE_REPORT_NUMBER_BUTTON)
> +	if (data[0] != KONEPURE_MOUSE_REPORT_NUMBER_BUTTON ||
> +	    size < sizeof(struct konepure_mouse_report_button))
>  		return 0;
>  
>  	if (konepure != NULL && konepure->roccat_claimed)
>  		roccat_report_event(konepure->chrdev_minor, data);

[Severity: High]
This is a pre-existing issue, but does calling roccat_report_event() here
lead to scheduling while atomic?

The raw_event handler runs in softirq context when the URB completion
handler calls hid_input_report(). However, roccat_report_event() acquires
sleeping locks:

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

Will this cause lockdep warnings or panics when those mutexes sleep?

[Severity: High]
This isn't a bug introduced by this patch, but while adding report size
validation for konepure, did we miss identical vulnerabilities in
hid-roccat-savu.c?

Looking at savu_raw_event(), it lacks report size validation before
processing the raw event buffer:

drivers/hid/hid-roccat-savu.c:savu_raw_event() {
    static int savu_raw_event(struct hid_device *hdev,
                              struct hid_report *report, u8 *data, int size)
    ...
}

Which then performs out-of-bounds struct field accesses on short reports:

drivers/hid/hid-roccat-savu.c:savu_report_to_chrdev() {
    ...
    special_report = (struct savu_mouse_report_special const *)data;

    roccat_report.type = special_report->type;
    roccat_report.data[0] = special_report->data[0];
    roccat_report.data[1] = special_report->data[1];
    ...
}

Could this lead to an out-of-bounds read if a malformed device sends a
shorter report, potentially leaking kernel memory to userspace?

[Severity: High]
This is a pre-existing issue, but is there a memory leak in roccat_release()
when bailing out early?

If a Roccat USB device is unplugged while a process has the character
device open, roccat_disconnect() sets devices[minor] = NULL but defers
freeing the device. Later, when the user closes it:

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

Does this early return skip freeing the reader struct and fail to decrement
device->open, permanently leaking both structures?

[Severity: High]
This is a pre-existing issue, but does destroying roccat_device leak
dynamically allocated report buffers?

When the device is finally destroyed, we free the device structure:

drivers/hid/hid-roccat.c:roccat_disconnect() {
    ...
    } else {
        kfree(device);
    }
    ...
}

And similarly in roccat_release():

drivers/hid/hid-roccat.c:roccat_release() {
    ...
    } else {
        kfree(device);
    }
    ...
}

Since roccat_report_event() allocates report data via kmemdup() into the
cbuf ring buffer, does calling kfree(device) without iterating over cbuf
leave those populated cbuf[i].value pointers leaked?

[Severity: High]
This isn't a bug introduced by this patch, but does a similar missing
size check exist in the Corsair VOID driver?

Looking at corsair_void_raw_event(), it processes status and firmware
reports by directly reading offsets up to data[4]:

drivers/hid/hid-corsair-void.c:corsair_void_raw_event() {
    ...
    if (hid_report->id == CORSAIR_VOID_STATUS_REPORT_ID) {
        drvdata->mic_up = FIELD_GET(CORSAIR_VOID_MIC_MASK, data[2]);
        drvdata->connected = (data[3] == CORSAIR_VOID_WIRELESS_CONNECTED) ||
                             drvdata->is_wired;
    ...
}

Without verifying that size is large enough, could a short report cause
an out-of-bounds memory read?

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