[PATCH] HID: rmi: check report length before trimming sentinel bytes

[PATCH] HID: rmi: check report length before trimming sentinel bytes

[Pengpeng Hou]

rmi_check_sanity() trims trailing 0xff sentinel bytes, but its loop
reads data[valid_size - 1] before checking that valid_size is non-zero.

Reverse the condition so the length is proved before the last byte is
inspected.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
 drivers/hid/hid-rmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index d4af17f..2bd781f 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c
@@ -365,7 +365,7 @@ static int rmi_check_sanity(struct hid_device *hdev, u8 *data, int size)
 	 * such reports here.
 	 */
 
-	while ((data[valid_size - 1] == 0xff) && valid_size > 0)
+	while (valid_size > 0 && data[valid_size - 1] == 0xff)
 		valid_size--;
 
 	return valid_size;

Re: [PATCH] HID: rmi: check report length before trimming sentinel bytes

[sashiko-bot]

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

Pre-existing issues:
- [High] `rmi_hid_write_block` writes data into a dynamically allocated heap buffer `data->writeReport` without verifying that the requested transfer length fits within the allocation.
- [High] `rmi_hid_read_block` trusts an attacker-controlled length field from the device report to read from the dynamically allocated `data->readReport` buffer, leading to an out-of-bounds read of kernel memory.
--

commit 5629c16e6055ea1ed7974ee398f60300987bc662
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>

HID: rmi: check report length before trimming sentinel bytes

rmi_check_sanity() trims trailing 0xff sentinel bytes, but its loop
reads data[valid_size - 1] before checking that valid_size is non-zero.

Reverse the condition so the length is proved before the last byte is
inspected.

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index d4af17fdba467..2bd781f1e0f53 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -365,7 +365,7 @@ static int rmi_check_sanity(struct hid_device *hdev, u8 *data, int size)
>  	 * such reports here.
>  	 */
>  
> -	while ((data[valid_size - 1] == 0xff) && valid_size > 0)
> +	while (valid_size > 0 && data[valid_size - 1] == 0xff)
>  		valid_size--;
>  
>  	return valid_size;

[Severity: High]
This is a pre-existing issue, but does this code overflow the dynamically
allocated data->writeReport buffer?

drivers/hid/hid-rmi.c:rmi_hid_write_block() {
    ...
	data->writeReport[2] = addr & 0xFF;
	data->writeReport[3] = (addr >> 8) & 0xFF;
	memcpy(&data->writeReport[4], buf, len);
    ...
}

A malicious HID device can present a small output_report_size in its HID
descriptor, which determines the size of the data->writeReport allocation.

By sending parameters that cause the RMI core to perform a large block write,
can the device force rmi_hid_write_block() to process a len that vastly
exceeds the allocated buffer size?

[Severity: High]
This isn't a bug introduced by this patch, but could this memcpy() result in
an out-of-bounds read from the data->readReport buffer?

drivers/hid/hid-rmi.c:rmi_hid_read_block() {
    ...
			read_input_count = data->readReport[1];
			memcpy(buf + bytes_read, &data->readReport[2],
				min(read_input_count, bytes_needed));
    ...
}

The function reads read_input_count directly from the incoming HID report
without validating it against the size of the data->readReport allocation
(data->input_report_size).

Could a malicious device specify a small input_report_size in its HID
descriptor but send a large read_input_count byte in the report, causing
the memcpy() to over-read the kernel heap?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/2026063007016439.1-ccfa108-0006-HID-rmi-check-report-length-pengpeng@iscas.ac.cn?part=1