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

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 [off-list ref]

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/ (local)
Fixes: 9fb6bf02e3ad ("HID: rmi: introduce RMI driver for Synaptics touchpads")
Cc: stable@vger.kernel.org

[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