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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing cancellation of `reset_work` in `rmi_probe()` error paths and incorrectly ordered cancellation in `rmi_remove()` leads to a Use-After-Free.
- [High] The timeout path in `rmi_hid_read_block()` fails to clear `RMI_READ_DATA_PENDING`, allowing a late reply to permanently desynchronize the request-retry state machine.
- [High] The `hdata->readReport` buffer and state flags are subject to a data race because `rmi_read_data_event()` writes to them concurrently without holding any locks that synchronize with `rmi_hid_read_block()`.
--

--- Patch [1]: [PATCH v5] HID: rmi: fix OOB access with undersized RMI reports ---
commit d795162d47703f2e01823d86b5a96b131c5a598a
Author: Wei Jie Law <98lawweijie@gmail.com>

HID: rmi: fix OOB access with undersized RMI reports

The patch fixes out-of-bounds reads and writes in hid-rmi by enforcing
minimum bounds on report sizes and handling zero-length replies correctly.
It also prevents the RMI_DEVICE flag from bypassing allocation on the error
paths.

Link: https://lore.kernel.org/linux-input/20260822121007.153988-1-98lawweijie@gmail.com/
Link: https://lore.kernel.org/linux-input/00a489f38b240624dcb5a4bae36a53fcba9cfb47.1787549195.git.98lawweijie@gmail.com/
Link: https://lore.kernel.org/linux-input/20260824122708.76168-1-98lawweijie@gmail.com/
Link: https://lore.kernel.org/linux-input/20260825060954.104890-1-98lawweijie@gmail.com/

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 2bd781f1e0f53..ecc19387f6b05 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -235,7 +235,23 @@ static int rmi_hid_read_block(struct rmi_transport_dev *xport, u16 addr,
>  				break;
>  			}

[Severity: High]
This is a pre-existing issue, but does the wait_event_timeout path just
above this hunk leave the RMI_READ_DATA_PENDING flag set?

If the timeout expires and the loop breaks, the exit block later clears
RMI_READ_REQUEST_PENDING but leaves RMI_READ_DATA_PENDING untouched. If a
late reply arrives from the device and triggers rmi_read_data_event(), it
will set RMI_READ_DATA_PENDING.

Would the next call to rmi_hid_read_block() immediately fall through
wait_event_timeout() and consume stale data?

> -			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 is there a data race between
this memcpy and rmi_read_data_event()?

While rmi_hid_read_block() is actively reading from data->readReport,
what happens if a glitchy device sends an unsolicited second reply?

Because RMI_READ_REQUEST_PENDING is still set for the duration of the
transaction, rmi_read_data_event() could concurrently overwrite
data->readReport. Also, clearing RMI_READ_DATA_PENDING here at the end of
the loop might erase the presence flag of that second reply. Is additional
synchronization needed between these paths?

[ ... ]
> @@ -696,6 +725,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 error path in rmi_probe() fail to
clean up the queued reset_work?

If hid_hw_start() fails (which is where this goto jumps to), and an event
has already arrived scheduling data->reset_work via rmi_event(), returning
an error here could cause a use-after-free when the un-cancelled work later
executes and accesses the freed hdata.

Similarly, in rmi_remove(), cancel_work_sync(&hdata->reset_work) is called
before hid_hw_stop(hdev). Could an incoming event schedule the work again
right after it is cancelled, since the hardware is still running?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825103117.12180-1-98lawweijie@gmail.com?part=1