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

[sashiko-bot@kernel.org]

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