Re: [PATCH 3/6] HID: roccat-pyra: reject short button reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free (UAF) in `pyra_raw_event` and `roccat_report_event` due to freeing device data before stopping the hardware.
- [Medium] Data race on `pyra->actual_profile` and `pyra->actual_cpi` between process context and atomic context.
- [Medium] Missing lock serialization across the multi-step device control sequence in sysfs read handlers.
- [Medium] The identical out-of-bounds read vulnerability fixed in `pyra` and other drivers in this series is still present in `savu`.
--

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

HID: roccat-pyra: reject short button reports

The Pyra raw-event path treats every button report as a complete
five-byte structure. A malformed USB device can send a shorter report
and make profile tracking or character-device event construction read
beyond the received input buffer.

> diff --git a/drivers/hid/hid-roccat-pyra.c b/drivers/hid/hid-roccat-pyra.c
> index 0d515995bb9d6..df8949c17ce3c 100644
> --- a/drivers/hid/hid-roccat-pyra.c
> +++ b/drivers/hid/hid-roccat-pyra.c
> @@ -557,6 +557,10 @@ static int pyra_raw_event(struct hid_device *hdev, struct hid_report *report,
>  	if (pyra == NULL)
>  		return 0;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
use-after-free to occur here during device removal?

In pyra_remove(), pyra_remove_specials() frees the device data via kfree()
before hid_hw_stop(hdev) is called. Because the hardware isn't stopped yet,
the USB interrupt endpoint remains active.

If an event is received in this narrow window, pyra_raw_event() fetches the
freed pointer locklessly. Since the pointer was never cleared, it passes
the NULL check and dereferences freed memory. Can we reorder the cleanup in
pyra_remove() to stop the hardware before freeing the data?

> +	if (data[0] == PYRA_MOUSE_REPORT_NUMBER_BUTTON &&
> +	    size < sizeof(struct pyra_mouse_event_button))
> +		return 0;
> +
>  	pyra_keep_values_up_to_date(pyra, data);

[Severity: Medium]
This is a pre-existing issue, but does pyra_keep_values_up_to_date() have a
data race with sysfs handlers?

The function pyra_sysfs_write_settings() runs in process context and modifies
shared state like pyra->actual_profile and pyra->actual_cpi under the
pyra->pyra_lock mutex. Meanwhile, pyra_raw_event() runs in atomic context and
modifies these same variables locklessly via pyra_keep_values_up_to_date().

An interrupt firing during a sysfs write could cause concurrent modifications,
leaving the variables in an inconsistent state.

>  	if (pyra->roccat_claimed)
>  		pyra_report_to_chrdev(pyra, data);
> 
>  	return 0;
>  }

[Severity: Medium]
This is also a pre-existing issue, but are the sysfs read handlers missing
lock serialization across multi-step device control sequences?

To read profile settings, handlers like pyra_sysfs_read_profilex_settings()
in drivers/hid/hid-roccat-pyra.c first send a control command via
pyra_send_control(), then read the data via pyra_sysfs_read(). The
pyra_send_control() call is unprotected, and pyra_sysfs_read() only acquires
pyra->pyra_lock for the receive operation.

If two threads read from different profiles concurrently, the send/receive
sequences can interleave (e.g., Thread 1 requests Profile 1, Thread 2 requests
Profile 2, Thread 1 receives data meant for Profile 2). Does the lock need to
cover the entire request-response transaction?

[Severity: Medium]
Finally, this is a pre-existing issue outside of the pyra driver, but does
the savu driver suffer from the same out-of-bounds read vulnerability being
fixed here?

While this patch adds size checks to pyra, savu_raw_event() in
drivers/hid/hid-roccat-savu.c still passes the unverified data buffer directly
to savu_report_to_chrdev(). The data is cast to struct
savu_mouse_report_special without ensuring it contains enough bytes.

If a malformed USB device sends a short report, it looks like this will leak
stale data from previous USB packets into the character device event stream.

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