Re: [PATCH 4/4] HID: roccat-savu: reject short special reports

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The driver acquires a mutex in the raw_event handler path, which executes in atomic (softIRQ) context, leading to a scheduling-while-atomic bug or deadlock.
- [Medium] The roccat driver leaks the memory allocated for circular buffer reports when the device is destroyed.
--

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

HID: roccat-savu: reject short special reports

savu_report_to_chrdev() casts special reports to a five-byte structure and
reads all of its payload fields without checking the received size. A
malformed USB device can therefore trigger out-of-bounds reads from the
input buffer when the character device is claimed. Pass the report size
into the helper and require the complete structure.

> diff --git a/drivers/hid/hid-roccat-savu.c b/drivers/hid/hid-roccat-savu.c
> index 6791369335601..04fa4c50cfa4b 100644
> --- a/drivers/hid/hid-roccat-savu.c
> +++ b/drivers/hid/hid-roccat-savu.c

[ ... ]

> @@ -183,7 +184,7 @@ static int savu_raw_event(struct hid_device *hdev,
>  		return 0;
>  
>  	if (savu->roccat_claimed)
> -		savu_report_to_chrdev(savu, data);
> +		savu_report_to_chrdev(savu, data, size);

[Severity: High]
This is a pre-existing issue, but can this lead to a scheduling-while-atomic
bug?

The savu_raw_event() callback runs in atomic context (softIRQ). When it
calls savu_report_to_chrdev(), the execution path proceeds to
roccat_report_event() in drivers/hid/hid-roccat.c, which acquires
sleeping locks:

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


[Severity: Medium]
This is also a pre-existing issue, but does this code leak the memory
allocated for circular buffer reports when the device is destroyed?

In drivers/hid/hid-roccat.c:roccat_report_event(), kmemdup() dynamically
allocates memory for the report:

    new_value = kmemdup(data, device->report_size, GFP_ATOMIC);

However, when the device is freed in roccat_release() or
roccat_disconnect() using kfree(device), the dynamically allocated report
values in the circular buffer do not appear to be cleaned up.

>  
>  	return 0;
>  }

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