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

[PATCH v3] 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 reads and writes fixed offsets into that buffer without
validating the sizes, and readReport is placed inside the same
allocation:

	data->readReport = data->writeReport + data->output_report_size;

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, while rmi_set_page() -- reached unconditionally at probe time through
rmi_input_configured() -- stores writeReport[4] and rmi_hid_read_block()
stores writeReport[0..5].  Since readReport is writeReport +
output_report_size, those stores land on top of the read buffer and
corrupt the window the next reply is parsed out of.

The read path is worse: 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 end of the allocation and into
adjacent slab objects.  This does not even need a lying device --
rmi_f01_probe() issues a fixed 21-byte register read, so any device
declaring an input report smaller than 23 bytes makes the driver read
out of bounds even when the device answers truthfully.  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 exported through the
mode 0444 sysfs attribute of the same name, 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 unprivileged
userspace and to the device itself.

The write path has no bound either: rmi_hid_write_block() copies an
unbounded len to &writeReport[4], and the largest caller a device can
drive at probe time is rmi_driver_set_irq_bits(), which passes
data->num_of_irq_regs -- derived from the interrupt source counts the
device declares in its Page Description Table, 39 entries per page over
as many pages as it likes.

Finally, the read loop cannot terminate on a zero-length reply: such a
reply copies nothing and advances neither bytes_read nor bytes_needed,
and because a reply did arrive the one second wait_event_timeout() does
not fire either, so a device answering 0 forever keeps the loop running
forever inside the probe worker with page_mutex held.  khungtaskd does
not notice, because every reply wakes the task and bumps its context
switch count.

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, clamp the write and the read copy to the report sizes
the device declared, and treat a zero-length reply as an error.  The
error path has to clear RMI_READ_DATA_PENDING on its way out, because
that flag is what the wait at the top of the loop tests: leaving it set
would make every later wait_event_timeout() return immediately on the
stale reply and kill the read path for the rest of the device's life.
Clamping the read count does not regress working hardware: the RMI read
loop already handles a reply carrying fewer bytes than requested, it
just goes round again.  A write longer than the output report was
overrunning the buffer already, so rejecting it cannot regress a device
that used to work.

Verified on v6.12.69 and on v6.12.105 built with CONFIG_KASAN=y and
booted kasan_multi_shot (generic KASAN otherwise reports only the first
error per boot), whose hid-rmi.c is identical to mainline here.  An
emulated RMI4 device driven over /dev/uhid, and the same device again
over dummy_hcd plus raw-gadget, give identical results:

  BUG: KASAN: slab-out-of-bounds in rmi_hid_read_block+0x409/0x750 [hid_rmi]
  Read of size 21 at addr ffff88800bf33bba by task kworker/0:3/285
  Workqueue: events uhid_device_add_worker
   kasan_report+0xc6/0x100
   kasan_check_range+0x105/0x1b0
   __asan_memcpy+0x23/0x60
   rmi_hid_read_block+0x409/0x750 [hid_rmi]
   rmi_f01_probe+0x5dd/0x1dc0 [rmi_core]

  BUG: KASAN: slab-out-of-bounds in rmi_hid_write_block+0x1a9/0x350 [hid_rmi]
  Write of size 35 at addr ffff88810a2b24ac by task kworker/1:10/666
   __asan_memcpy+0x3c/0x60
   rmi_hid_write_block+0x1a9/0x350 [hid_rmi]
   rmi_driver_set_irq_bits+0x1f6/0x4d0 [rmi_core]
   rmi_f30_config+0x27a/0x4d0 [rmi_core]
   rmi_driver_process_config_requests+0xe7/0x150 [rmi_core]
   rmi_driver_probe+0x636/0xbf0 [rmi_core]
   rmi_register_transport_device+0x19e/0x3e0 [rmi_core]
   rmi_input_configured+0x184/0x2e0 [hid_rmi]
   hidinput_connect+0x13e3/0x2a50
   hid_hw_start+0x89/0x120
   rmi_probe+0x952/0xcf0 [hid_rmi]

and for the zero-length reply, after 225 replies at 200 ms intervals:

  kworker/1:0+events  state=D
   rmi_hid_read_block+0x5cc/0x750 [hid_rmi]
   rmi_scan_pdt+0x211/0x3f0 [rmi_core]
   rmi_driver_probe+0x1c0/0xbf0 [rmi_core]
   really_probe+0x1e3/0x930

After this change the undersized descriptor is refused at probe with
"rmi reports too small (out=2 in=2)", the oversized read and write are
both rejected, the zero-length reply fails the read with -EIO and the
worker returns while later reads on the same device keep working, and a
device declaring reports large enough for a 21-byte register read still
probes normally and reports its real product id.

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/
Fixes: 9fb6bf02e3ad ("HID: rmi: introduce RMI driver for Synaptics touchpads")
Cc: stable@vger.kernel.org
Signed-off-by: Wei Jie Law <98lawweijie@gmail.com>
---
Changes in v3:
 - Clear RMI_READ_DATA_PENDING before bailing out of the read loop on a
   zero-length reply.  v2 left the flag set, and that flag is what the
   wait at the top of the loop tests, so every later
   wait_event_timeout() returned immediately on the stale reply: the
   four remaining retries of that call, and every subsequent
   rmi_hid_read_block(), failed instantly with -EIO without ever waiting
   for the device again.  One zero-length reply from an otherwise honest
   device was enough to kill the read path for the rest of the device's
   life.  Against a device that went silent after the zero-length reply,
   v2 still finished all five retries in 54 us.
 - Express the output-report bound as "len + 4 > output_report_size"
   instead of "len > output_report_size - 4".  Both report sizes are
   u32, so the subtraction form is only safe because of the probe-time
   minimum this patch also adds; this form does not lean on it.
 - No other functional change; the three checks from v1 are as they were.

Changes in v2:
 - Corrected the claim in the v1 commit message that rmi_set_page()
   writes one byte past the allocation.  That has not been true since
   commit 6fcd7e702d3d ("devres: Use kmalloc_size_roundup() to match
   ksize() usage"), which makes check_dr_size() round the devres
   allocation up to the whole kmalloc bucket, so devm_kzalloc(4) is a
   64-byte kmalloc and the store at offset 44 is in bounds -- KASAN
   stays silent on it, correctly.  What that store does do is land on
   top of readReport, since readReport is writeReport +
   output_report_size.  The out-of-bounds accesses are the read and the
   unbounded write, and the KASAN reports for both are now quoted.
 - Reject a zero-length READ_DATA reply.  Such a reply advances neither
   bytes_read nor bytes_needed, and because a reply did arrive the
   wait_event_timeout() does not fire either, so a device answering 0
   forever spins in rmi_hid_read_block() indefinitely inside the probe
   worker with page_mutex held.  The v1 clamp does not help, since
   min_t(int, 0, input_report_size - 2) is still 0.  khungtaskd does not
   notice because every reply wakes the task.
 - No functional change to the three checks already in v1.
v1: https://lore.kernel.org/linux-input/20260822121007.153988-1-98lawweijie@gmail.com/
v2: https://lore.kernel.org/linux-input/00a489f38b240624dcb5a4bae36a53fcba9cfb47.1787549195.git.98lawweijie@gmail.com/

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

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index d4af17fdba46..cfbd5a245283 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;
 			}
 
-			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__);
+				clear_bit(RMI_READ_DATA_PENDING, &data->flags);
+				ret = -EIO;
+				break;
+			}
 			memcpy(buf + bytes_read, &data->readReport[2],
 				min(read_input_count, bytes_needed));
 
@@ -271,6 +287,11 @@ static int rmi_hid_write_block(struct rmi_transport_dev *xport, u16 addr,
 			goto exit;
 	}
 
+	if (len + 4 > data->output_report_size) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
 	data->writeReport[0] = RMI_WRITE_REPORT_ID;
 	data->writeReport[1] = len;
 	data->writeReport[2] = addr & 0xFF;
@@ -696,6 +717,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;
 
-- 
2.43.0

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

[sashiko-bot]

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

New issues:
- [High] NULL pointer dereference introduced by the patch in rmi_probe().

Pre-existing issues:
- [High] Use-After-Free due to improper teardown order in rmi_remove().
- [High] Resource leak and Use-After-Free in rmi_probe error path.
- [High] TOCTOU race condition in chunked reads causes data corruption.
- [High] Global state pollution of rmi_hid_pdata in rmi_probe.
--

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

HID: rmi: fix OOB access with undersized RMI reports

The hid-rmi driver sizes its writeReport/readReport buffer purely from
the report descriptor supplied by the device, with no minimum bound.
This patch adds bounds checking to prevent out-of-bounds accesses during
reads and writes.

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/

> diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
> index 2bd781f1e0f53..21076521e35e7 100644
> --- a/drivers/hid/hid-rmi.c
> +++ b/drivers/hid/hid-rmi.c

[ ... ]

> @@ -696,6 +717,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;
> +	}

[Severity: High]
Does this goto bypass the allocation of data->writeReport?

If a device is dynamically bound using the new_id sysfs interface, and the
driver_data provided has BIT(0) (RMI_DEVICE) set, data->device_flags will
already have RMI_DEVICE set from id->driver_data earlier in rmi_probe().

When this jumps to the start label, hid_hw_start() will call
rmi_input_configured() which checks the RMI_DEVICE flag. Since it is set,
it will proceed to call rmi_set_page(), which dereferences
data->writeReport[0].

Since data->writeReport was never allocated, could this cause a NULL pointer
dereference?

>  	data->device_flags |= RMI_DEVICE;
>  	alloc_size = data->output_report_size + data->input_report_size;
>  


[Severity: High]
This is a pre-existing issue, but is there a use-after-free in rmi_remove()
due to the teardown order?

In rmi_remove(), cancel_work_sync(&hdata->reset_work) is called before
hid_hw_stop(hdev). Since hid_hw_stop() is what ultimately stops incoming
hardware events, a hardware interrupt could still occur after the worker is
cancelled.

This incoming event invokes rmi_event(), which unconditionally calls:

    schedule_work(&data->reset_work);
    return 1;

Once rmi_remove() returns, devres cleanup frees the hdata structure. The
newly scheduled work item could then execute rmi_reset_work() and
dereference the freed hdata memory.


[Severity: High]
This is a pre-existing issue, but does the rmi_probe() error path leak the
transport device?

During rmi_probe(), hid_hw_start() is called, which invokes the driver's
rmi_input_configured() callback. This callback registers the transport
device:

    ret = rmi_register_transport_device(&data->xport);

If hid_hw_start() subsequently fails (for example, during hidraw
registration), rmi_probe() returns the error without unregistering the
transport device:

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

Devres will free the hdata structure, but the RMI core still retains a
pointer to the now-freed hdata->xport device, which could lead to a
use-after-free.


[Severity: High]
This is a pre-existing issue, but is there a race condition in chunked reads
that could cause data corruption?

When rmi_hid_read_block() requests a read larger than what fits in a single
input report, the device responds with multiple consecutive reports. Each
report triggers rmi_read_data_event() in interrupt context, which executes:

    memcpy(hdata->readReport, data,
           min((u32)size, hdata->input_report_size));
    set_bit(RMI_READ_DATA_PENDING, &hdata->flags);
    wake_up(&hdata->wait);

There appears to be no synchronization to prevent a subsequent report from
overwriting hdata->readReport before the consumer thread in
rmi_hid_read_block() wakes up to consume the previous chunk:

    while (bytes_read < len) {
        if (!wait_event_timeout(data->wait,
            test_bit(RMI_READ_DATA_PENDING, &data->flags),

If the device sends reports faster than the thread gets scheduled, could
data be overwritten and silently lost?


[Severity: High]
This is a pre-existing issue, but does rmi_probe() modify global state
instead of device-specific state?

rmi_hid_pdata is defined as a static global template structure. However,
in rmi_probe(), if the device has physical buttons, it modifies the global
template directly:

    if (data->device_flags & RMI_DEVICE_HAS_PHYS_BUTTONS)
        rmi_hid_pdata.gpio_data.disable = true;

    data->xport.dev = hdev->dev.parent;
    data->xport.pdata = rmi_hid_pdata;

This permanently mutates the global template. Could all subsequently probed
devices erroneously inherit gpio_data.disable = true, even if they do not
have physical buttons?

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