Re: [PATCH 2/6] HID: roccat-isku: reject short button reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `.raw_event` callback acquires mutexes while running in an atomic context, leading to a "sleeping function called from invalid context" bug. The HID core invokes the raw event handlers (`isku_raw_event`, etc.) from the USB URB completion handler, which executes in atomic context (softirq). The event handler delegates to `roccat_report_event()`, which erroneously acquires two mutexes.
- [High] Improper teardown ordering allows the URB completion handler to access freed driver data (Use-After-Free). In `isku_remove()` and the error path of `isku_probe()`, `isku_remove_specials()` is called before `hid_hw_stop()`. This frees the `isku` structure but leaves the hardware URB polling active. If a USB interrupt completes in this window, `isku_raw_event()` fetches the freed `isku` pointer via `hid_get_drvdata(hdev)` and dereferences it.
- [Medium] The patch fixes an out-of-bounds read in `isku_raw_event` caused by missing size validation, and subsequent patches in the series fix similar flaws in `koneplus`, `konepure`, `kovaplus`, `pyra`, and `ryos`. However, the `savu` driver was left unpatched. In `savu_raw_event()`, if a malformed USB device issues a truncated packet, the driver reads fields directly from the uninitialized portion of the HID core's URB buffer (out-of-bounds of the actual packet) and forwards this data to the character device, leaking kernel memory to user space.
--

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

HID: roccat-isku: reject short button reports

The Isku raw-event path casts button reports to a five-byte structure
and reads the event payload without validating the received size. A
malformed USB device can therefore trigger out-of-bounds reads from a
short report. Require the complete button report before updating or
forwarding it.

> diff --git a/drivers/hid/hid-roccat-isku.c b/drivers/hid/hid-roccat-isku.c
> index 93a49c93ae8c0..c65f414b13cdf 100644
> --- a/drivers/hid/hid-roccat-isku.c
> +++ b/drivers/hid/hid-roccat-isku.c
> @@ -411,6 +411,10 @@ static int isku_raw_event(struct hid_device *hdev,
>  	if (isku == NULL)
>  		return 0;
>  
> +	if (data[0] == ISKU_REPORT_NUMBER_BUTTON &&
> +	    size < sizeof(struct isku_report_button))
> +		return 0;
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but since we are fixing
out-of-bounds reads here, should a similar size validation be applied to the
savu driver?

In drivers/hid/hid-roccat-savu.c:savu_raw_event(), if a malformed USB device
issues a truncated packet, the driver passes input data without size
validation:

    if (savu->roccat_claimed)
        savu_report_to_chrdev(savu, data);

Then savu_report_to_chrdev() reads fields directly from the uninitialized
portion of the HID core's URB buffer:

    special_report = (struct savu_mouse_report_special const *)data;
    roccat_report.type = special_report->type;

Could this leak kernel memory to user space?

>  	isku_keep_values_up_to_date(isku, data);

[Severity: High]
This is a pre-existing issue, but does the teardown order in isku_remove()
allow a use-after-free here if an interrupt fires during device disconnect?

In isku_remove() and the probe error path, the driver data is freed via
isku_remove_specials() before stopping the hardware URBs:

    isku_remove_specials(hdev);
    hid_hw_stop(hdev);

If a final URB completes in this window, isku_raw_event() fetches the freed
isku pointer and dereferences it in isku_keep_values_up_to_date().

>  
>  	if (isku->roccat_claimed)

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

The HID core invokes isku_raw_event() from the USB URB completion handler
in softirq context. This delegates to isku_report_to_chrdev() and then to
roccat_report_event(), which acquires mutexes:

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

Can this cause a "sleeping function called from invalid context" bug?

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