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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The device's read state machine becomes permanently poisoned because the new zero-length reply error path leaks the `RMI_READ_DATA_PENDING` flag.

Pre-existing issues:
- [High] Use-After-Free occurs because `reset_work` can execute after `rmi_data` is freed, due to improper cleanup ordering in `rmi_remove()` and missing cancellation in the `probe()` error path.
- [High] Subsystem guide violation: `rmi_report()` accesses report fields by index without validating the field count first.
- [Medium] Data corruption can occur because `hdata->readReport` is modified concurrently by the IRQ handler while being read by the process context.
--

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

HID: rmi: fix OOB access with undersized RMI reports

This commit addresses out-of-bounds read and write accesses in the hid-rmi
driver caused by undersized RMI reports. It introduces size validation during
probe and clamps read/write lengths to the device-declared sizes. It also
treats zero-length read replies as errors to prevent an infinite loop.

Link: https://lore.kernel.org/linux-input/20260822121007.153988-1-98lawweijie@gmail.com/
Fixes: 9fb6bf02e3ad ("HID: rmi: introduce RMI driver for Synaptics touchpads")
Cc: stable@vger.kernel.org

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c
> @@ -235,7 +235,22 @@ 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);
> +			if (!read_input_count) {
> +				/*
> +				 * A zero length reply advances neither
> +				 * bytes_read nor bytes_needed, and because a
> +				 * reply did arrive the wait above does not
> +				 * time out either, so a device answering 0
> +				 * forever would spin here indefinitely with
> +				 * page_mutex held.
> +				 */
> +				hid_warn(hdev, "%s: zero-length read reply\n",
> +					 __func__);
> +				ret = -EIO;
> +				break;
> +			}

[Severity: High]
If a zero-length reply is received and we break here, does this bypass the
clear_bit(RMI_READ_DATA_PENDING, &data->flags) at the end of the loop? It
looks like leaving this flag set might permanently poison the state machine,
causing subsequent loops or calls to rmi_hid_read_block() to pass the wait
condition immediately and fail with -EIO.

>  			memcpy(buf + bytes_read, &data->readReport[2],
>  				min(read_input_count, bytes_needed));

[Severity: Medium]
This is a pre-existing issue, but could this memcpy race with the IRQ
handler? While rmi_hid_read_block() is reading from data->readReport,
rmi_read_data_event() might concurrently overwrite hdata->readReport if an
unsolicited report arrives, potentially leading to corrupted register reads.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but can a use-after-free occur
during device removal? In rmi_remove(), the work is cancelled before hardware
events are stopped:

	cancel_work_sync(&hdata->reset_work);
	rmi_unregister_transport_device(&hdata->xport);
}
hid_hw_stop(hdev);

Because hid_hw_stop() is called after cancel_work_sync(), incoming events
can still arrive after the work is cancelled. If rmi_event() queues the work
again via schedule_work(&data->reset_work), could it execute after devres
frees the rmi_data structure? Additionally, if hid_hw_start() fails in
rmi_probe(), it appears to return without cancelling the work.

[Severity: High]
This is a pre-existing issue, but does rmi_report() lack report field
validation?

	struct hid_field *field = report->field[0];

The HID subsystem guidelines state that accessing report fields by index
without first checking that report->maxfield is at least 1 can lead to
out-of-bounds accesses with crafted descriptors.

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