[PATCH] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer

[PATCH] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

rmi_f54_work() reads a diagnostics report from the device into
f54->report_data, sizing the transfer with rmi_f54_get_report_size():

	report_size = rmi_f54_get_report_size(f54);
	...
	for (i = 0; i < report_size; i += F54_REPORT_DATA_SIZE) {
		int size = min(F54_REPORT_DATA_SIZE, report_size - i);
		...
		rmi_read_block(.., f54->report_data + i, size);
	}

report_data is allocated once at probe from F54's own electrode counts
(array3_size(f54->num_tx_electrodes, f54->num_rx_electrodes, sizeof(u16))),
but rmi_f54_get_report_size() computes the size from
drv_data->num_*_electrodes when those are set, i.e. from the F55
function's electrode counts. Both counts come straight from device
queries (F54 and F55 each report up to 255 electrodes) and nothing
constrains the F55 counts to the F54 ones.

A malicious or malfunctioning RMI4 device that reports larger F55
electrode counts than its F54 counts makes report_size exceed the
allocation, so the read loop writes past report_data (and the V4L2
dequeue memcpy() then reads past it). On conforming hardware the F55
configured electrodes are a subset of the F54 physical electrodes, so
report_size never exceeds the buffer and well-behaved devices are
unaffected.

Record the allocation size and reject a report that does not fit,
mirroring the existing zero-size check.

Fixes: c762cc68b6a1 ("Input: synaptics-rmi4 - propagate correct number of rx and tx electrodes to F54")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Reachable from a malicious or counterfeit RMI4 peripheral (HID/I2C/SPI)
that exposes both an F54 diagnostics function and an F55 sensor-tuning
function reporting more electrodes than F54; the report is filled when a
local client drives an F54 capture through the V4L2 touch device node.

Verified with an in-kernel KASAN litmus that allocates report_data from
the F54 dims and fills it from the F55-preferred dims (the verbatim
rmi_f54_get_report_size() preference and the rmi_f54_work() 32-byte read
loop), Linux 7.1.0-rc5, CONFIG_KASAN=y, kasan.fault=panic, x86_64:

  Arm A, F54 8x8 (capacity 128) vs F55 8x24 (report_size 384):
    BUG: KASAN: slab-out-of-bounds in ... Write of size 32
    located 0 bytes to the right of the allocated 128-byte region
    ... cache kmalloc-128 of size 128
    Kernel panic - not syncing: kasan.fault=panic set
  Arm B, with this patch (oversize report rejected): clean
  Arm C, conforming device (F55 dims == F54 dims): clean

  AddressSanitizer (x86_64 and i386): heap-buffer-overflow WRITE of size 32,
  0 bytes after the 128-byte region.

The same rmi_f54_get_report_size() value drives the V4L2 dequeue
memcpy(.., report_size) on the read side, so an oversize report also
over-reads report_data; rejecting it in rmi_f54_work() (report_size then
stays 0) closes both. Reproducer and full logs available on request.

This is independent of the in-flight "Input: rmi4 - release F54 queue on
video registration failure" change, which touches only the probe error
path (vb2_queue_release in rmi_f54_probe); it applies cleanly alongside it.
---
 drivers/input/rmi4/rmi_f54.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 61909e1a39e2..7a7508a4de85 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -104,6 +104,7 @@ struct f54_data {
 
 	enum rmi_f54_report_type report_type;
 	u8 *report_data;
+	size_t report_data_size;
 	int report_size;
 
 	bool is_busy;
@@ -548,6 +549,13 @@ static void rmi_f54_work(struct work_struct *work)
 		goto error;     /* retry won't help */
 	}
 
+	if (report_size > f54->report_data_size) {
+		dev_err(&fn->dev, "Report size %d exceeds buffer size %zu\n",
+			report_size, f54->report_data_size);
+		error = -EINVAL;
+		goto error;
+	}
+
 	/*
 	 * Need to check if command has completed.
 	 * If not try again later.
@@ -678,8 +686,8 @@ static int rmi_f54_probe(struct rmi_function *fn)
 
 	rx = f54->num_rx_electrodes;
 	tx = f54->num_tx_electrodes;
-	f54->report_data = devm_kzalloc(&fn->dev,
-					array3_size(tx, rx, sizeof(u16)),
+	f54->report_data_size = array3_size(tx, rx, sizeof(u16));
+	f54->report_data = devm_kzalloc(&fn->dev, f54->report_data_size,
 					GFP_KERNEL);
 	if (f54->report_data == NULL)
 		return -ENOMEM;

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260613-b4-disp-6721686c-33a3bdacdf38

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Stale f54->report_size causes V4L2 buffer heap overflow.
- [High] Missing cancel_delayed_work_sync in rmi_f54_remove causes Use-After-Free.
- [High] Preexisting copy-paste typo in F55 properties parsing triggers the new oversize check, permanently breaking conforming devices.
- [High] Use-After-Free of devm-managed video_device on V4L2 file descriptor close.
- [Medium] V4L2 buffer queue silently returns uninitialized memory on hardware errors.
--

commit 08992eb2bae1821355b21e17559ef51ec82606cd
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer

This commit adds a bounds check to ensure the F54 diagnostic report size
does not exceed the allocated buffer. It rejects reports that are larger
than the allocation to prevent writing past the end of the report data
buffer.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 61909e1a39e24..7a7508a4de85c 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
[ ... ]
> @@ -548,6 +549,13 @@ static void rmi_f54_work(struct work_struct *work)
>  		goto error;     /* retry won't help */
>  	}
>  
> +	if (report_size > f54->report_data_size) {

[Severity: High]
This is a pre-existing issue, but does this new check permanently break
conforming hardware due to a copy-paste typo in the F55 parser?

Looking at rmi_f55_detect() in rmi_f55.c, it incorrectly assigns the
RX count to the TX count:

drv_data->num_rx_electrodes = f55->cfg_num_rx_electrodes;
drv_data->num_tx_electrodes = f55->cfg_num_rx_electrodes;

For hardware lacking F55_CAP_SENSOR_ASSIGN and having more RX electrodes
than TX electrodes, this inflates the perceived TX count. This inflated count
propagates to rmi_f54_get_report_size(), yielding a larger value that will
unconditionally fail this new bounds check.

> +		dev_err(&fn->dev, "Report size %d exceeds buffer size %zu\n",
> +			report_size, f54->report_data_size);
> +		error = -EINVAL;
> +		goto error;

[Severity: Critical]
This isn't a bug introduced by this patch, but does taking this goto error
path bypass the zeroing of f54->report_size and cause a V4L2 buffer heap
overflow?

In rmi_f54_work(), error paths jump to the error: label, bypassing the
abort: label where f54->report_size is normally zeroed out. This leaves
f54->report_size containing its previous successful payload size. 

If a user then alters the V4L2 format to a smaller size, and an error occurs
(like this bounds check failing), rmi_f54_buffer_queue() will copy the
stale, larger payload size into the shrunken V4L2 buffer:

memcpy(ptr, f54->report_data, f54->report_size);

[Severity: Medium]
This is also a pre-existing issue, but does this error path cause the V4L2
buffer queue to silently return uninitialized memory?

When rmi_f54_work() triggers this bounds check and jumps to error, the
buffer is not updated. However, rmi_f54_buffer_queue() ignores the failure
and marks the buffer as fully populated:

vb2_set_plane_payload(vb, 0, rmi_f54_get_report_size(f54));
state = VB2_BUF_STATE_DONE;

This completes the buffer without indicating an error to userspace.

> +	}
> +
>  	/*
>  	 * Need to check if command has completed.
[ ... ]
> @@ -678,8 +686,8 @@ static int rmi_f54_probe(struct rmi_function *fn)
>  
>  	rx = f54->num_rx_electrodes;
>  	tx = f54->num_tx_electrodes;
> -	f54->report_data = devm_kzalloc(&fn->dev,
> -					array3_size(tx, rx, sizeof(u16)),
> +	f54->report_data_size = array3_size(tx, rx, sizeof(u16));
> +	f54->report_data = devm_kzalloc(&fn->dev, f54->report_data_size,
>  					GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does allocating the f54_data struct
(which embeds struct video_device vdev) with devm_kzalloc lead to a
use-after-free when the V4L2 file descriptor is closed?

In rmi_f54_probe(), f54 is allocated using devm_kzalloc(). When the driver
unbinds, devm automatically frees f54_data. 

If a userspace application still holds /dev/videoX open, the V4L2 core
retains a reference to the freed vdev. Subsequent ioctl calls or the final
closing of the file descriptor will access the freed vdev structure.

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of a timer
cancellation in rmi_f54_remove() cause a use-after-free?

While rmi_f54_probe() correctly uses cancel_delayed_work_sync(&f54->work)
in its error path, the teardown path in rmi_f54_remove() just destroys
the workqueue:

rmi_f54_remove() {
    ...
    video_unregister_device(&f54->vdev);
    v4l2_device_unregister(&f54->v4l2);
    destroy_workqueue(f54->workqueue);
    ...
}

This does not cancel pending delayed_work timers. If the driver is unbound
while a V4L2 stream is active, the timer will fire after devm has freed
the f54 structure and attempt to dereference f54->workqueue.

>  	if (f54->report_data == NULL)
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-b4-disp-6721686c-v1-1-4d5bb84ee520@proton.me?part=1