Re: [PATCH] HID: rmi: fix OOB access with undersized RMI reports

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

Pre-existing issues:
- [High] Missing report field count validation before access.
- [High] Use-After-Free of `data->reset_work` on `probe` error path.
- [High] Data Race and missed wakeups in `rmi_hid_read_block`.
--

--- Patch [1]: [PATCH] HID: rmi: fix OOB access with undersized RMI reports ---
commit 723e0f956aac9b958efffab91d1bca6e5f8025a4
Author: Wei Jie Law <[email protected]>

HID: rmi: fix OOB access with undersized RMI reports

This patch adds minimum bound checks for the read/write report buffers
in hid-rmi.c to prevent out-of-bounds access. It rejects reports that
are too small at probe time.

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index d4af17fdba467..a0780087d413f 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -235,7 +235,8 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  				break;
>  			}
>  
> -			read_input_count = data->readReport[1];
> +			read_input_count = min_t(int, data->readReport[1],
> +						 data->input_report_size - 2);
>  			memcpy(buf + bytes_read, &data->readReport[2],
>  				min(read_input_count, bytes_needed));
>  
>  			bytes_read += read_input_count;
>  			bytes_needed -= read_input_count;
>  			clear_bit(RMI_READ_DATA_PENDING, &data->flags);

[Severity: High]
This isn't a bug introduced by this patch, but does this read path have a data
race and missed wakeups?

The data->readReport buffer is read here by process context, but it appears to
be written concurrently from interrupt context without synchronization:

drivers/hid/hid-rmi.c:rmi_read_data_event() {
    ...
    memcpy(hdata->readReport, data, min((u32)size, hdata->input_report_size));
    set_bit(RMI_READ_DATA_PENDING, &hdata->flags);
}

If the device streams multiple input reports consecutively, can the softirq
overwrite data->readReport while it is being copied in rmi_hid_read_block()?

Furthermore, since clear_bit(RMI_READ_DATA_PENDING) runs after the copy,
could it erroneously clear the flag for a new chunk if the softirq sets it
just before clear_bit executes, causing the process context to wait and
timeout?

[ ... ]

> @@ -696,6 +702,17 @@ static int rmi_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	data->output_report_size = hid_report_len(output_report);
>  
> +	/*
> +	 * The write reports built by this driver occupy 6 bytes and the read
> +	 * handshake looks at the first 3 bytes of an input report, so refuse
> +	 * to drive a device whose reports cannot hold them.
> +	 */
> +	if (data->output_report_size < 6 || data->input_report_size < 3) {
> +		hid_err(hdev, "rmi reports too small (out=%u in=%u)\n",
> +			data->output_report_size, data->input_report_size);
> +		goto start;
> +	}
> +
>  	data->device_flags |= RMI_DEVICE;
>  	alloc_size = data->output_report_size + data->input_report_size;
>  

[Severity: High]
This is a pre-existing issue, but does the probe error path contain a
use-after-free of data->reset_work?

Down at the start label, the driver calls hid_hw_start():

start:
    ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
    if (ret) {
        hid_err(hdev, "hw start failed\n");
        return ret;
    }

During hid_hw_start(), the core allows incoming reports. If a subsequent step
fails (e.g., rmi_set_mode()), hid_hw_start() returns an error.

If incoming reports have already triggered rmi_event() and scheduled
data->reset_work, returning this error will cause devres to free the data
structure. Could the workqueue then execute rmi_reset_work() on freed memory
since the pending work is not cancelled before returning?

[Severity: High]
Additionally, there is another pre-existing issue in rmi_report(). Does it
access the field array without verifying the report field count?

drivers/hid/hid-rmi.c:rmi_report() {
    ...
    struct hid_field *field = report->field[0];
    ...
}

The code accesses report->field[0] by index. If a malicious or buggy device
provides a report descriptor with no fields (making report->maxfield 0),
would this lead to an out-of-bounds access?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1