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

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

[Wei Jie Law]

The hid-rmi driver sizes its writeReport/readReport buffer purely from
the report descriptor supplied by the device, with no minimum bound:

	data->input_report_size  = hid_report_len(input_report);
	data->output_report_size = hid_report_len(output_report);
	alloc_size = data->output_report_size + data->input_report_size;
	data->writeReport = devm_kzalloc(&hdev->dev, alloc_size, GFP_KERNEL);

but then writes fixed offsets into that buffer without validating the
sizes.  A device declaring a 1-byte output report (0x09) and a 1-byte
input report (0x0c) makes hid_report_len() return 2 for each, so
alloc_size is 4, and rmi_set_page() -- reached unconditionally at probe
time through rmi_input_configured() -- writes writeReport[4], one byte
past the allocation.  rmi_hid_read_block() writes writeReport[0..5] in
the same way, and rmi_hid_write_block() copies an unbounded len to
&writeReport[4].

The read path has the mirror image of the problem: the copy length
comes from readReport[1], which is filled in from the device's response
and can be up to 255, and the copy starts at &readReport[2] without any
regard for input_report_size, so it runs past the allocation and into
adjacent slab objects.  Those bytes become the register values the RMI
core acts on; they are printed to the kernel log as the product id by
rmi_f01_probe(), and they are sent back to the device as the interrupt
mask by rmi_driver_set_irq_bits(), so an undersized report descriptor
leaks heap contents both to userspace and to the device itself.

Reject reports that are too small at probe time, where the driver needs
6 output bytes for the write reports it builds and 3 input bytes for the
read handshake, and clamp the write and the read copy to the report
sizes the device declared.  A write longer than the output report was
already overrunning the buffer, so rejecting it cannot regress a device
that used to work.

Verified on v6.12.104, whose hid-rmi.c is identical to mainline here,
with a UHID reproducer and with an emulated USB device (raw-gadget): a
breakpoint on rmi_set_page() shows the store to writeReport[4] executing
with alloc_size == 4, and after the change probe stops with "rmi reports
too small (out=2 in=2)".  With a larger descriptor the read path returns
kernel heap bytes, observed both in the product id printed by
rmi_f01_probe() and in the interrupt mask written back to the emulated
device.

Fixes: 9fb6bf02e3ad ("HID: rmi: introduce RMI driver for Synaptics touchpads")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
The reproducer is available on request.

Compile-tested on bd5f485f3f02, x86_64 defconfig + CONFIG_HID_RMI=y.

 drivers/hid/hid-rmi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index d4af17fdba46..a0780087d413 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));
 
@@ -271,6 +272,11 @@ static int rmi_hid_write_block(struct rmi_transport_dev *xport, u16 addr,
 			goto exit;
 	}
 
+	if (len > data->output_report_size - 4) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	data->writeReport[0] = RMI_WRITE_REPORT_ID;
 	data->writeReport[1] = len;
 	data->writeReport[2] = addr & 0xFF;
@@ -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;
 

base-commit: bd5f485f3f026225b86573e559af0b7254ef4184
-- 
2.43.0

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

[sashiko-bot]

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 <98lawweijie@gmail.com>

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/20260822121007.153988-1-98lawweijie@gmail.com?part=1

Re: [PATCH v1] Input: rmi4 - fix out-of-bounds write in rmi_set_page

[Wei Jie Law]

Superseded by v2:
https://lore.kernel.org/linux-input/00a489f38b240624dcb5a4bae36a53fcba9cfb47.1787549195.git.98lawweijie@gmail.com/

v1's commit message claims rmi_set_page() writes one byte past the
allocation. That is wrong on any kernel since v6.1 -- check_dr_size()
rounds the devres allocation up to the whole kmalloc bucket, so
devm_kzalloc(4) is a 64-byte kmalloc and that store is in bounds. v2
corrects the reasoning and adds one more fix to the same loop.