Re: [PATCH] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer
From: sashiko-bot@kernel.org
Date: 2026-06-14 04:14:31
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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