Re: [PATCH 06/10] Input: synaptics-rmi4 - propagate F54 worker errors to V4L2 queue

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The V4L2 video_device, vb2_queue, and mutex lock are embedded inside struct f54_data, which is allocated using devm_kzalloc(). When the device is unbound, devres frees f54_data immediately after rmi_f54_remove() returns. If a userspace process holds the /dev/videoX file descriptor open, subsequent V4L2 ioctl calls or close() operations will trigger a Use-After-Free.
- [Medium] The V4L2 buf_queue callback executes synchronous, blocking hardware polling via wait_for_completion_timeout(), turning an asynchronous queueing mechanism into a fully blocking operation that starves other ioctls.
--

commit ed5dda6c6cb42efd8468fff80b16a819c0e59afb
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: synaptics-rmi4 - propagate F54 worker errors to V4L2 queue

Previously, rmi_f54_buffer_queue() waited for the worker thread to finish but
ignored whether it succeeded. This patch adds a report_error field to struct
f54_data to store the worker's exit status and marks the buffer with
VB2_BUF_STATE_ERROR if the worker failed.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index e86dfc9ce7d9a..c86bc81845bb1 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -106,6 +106,7 @@ struct f54_data {
>  	u8 *report_data;
>  	size_t max_report_size;
>  	int report_size;
> +	int report_error;

[Severity: High]
This is a pre-existing issue, but does using devres to allocate struct
f54_data create a use-after-free risk during device removal?

Since the V4L2 video_device, vb2_queue, and mutex lock are embedded inside
this structure, and it is allocated via devm_kzalloc() in rmi_f54_probe():

rmi_f54_probe() {
        ...
        f54 = devm_kzalloc(&fn->dev, sizeof(struct f54_data), GFP_KERNEL);
        ...
}

The memory will be freed immediately when the device is unbound. If a
userspace process still holds the /dev/videoX file descriptor open,
subsequent V4L2 ioctl calls or close() operations could access the freed
video_device or queue.

[ ... ]

> @@ -340,6 +341,12 @@ static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
>  		mutex_lock(&f54->data_mutex);
>  	}
>  
> +	if (f54->report_error) {
> +		dev_err(&f54->fn->dev, "Error acquiring report: %d\n", f54->report_error);
> +		state = VB2_BUF_STATE_ERROR;
> +		goto data_done;
> +	}
> +

[Severity: Medium]
This isn't a bug introduced by this patch, but does waiting for the hardware
completion inside rmi_f54_buffer_queue() starve other concurrent V4L2
operations?

Just before this error check, rmi_f54_buffer_queue() executes:

rmi_f54_buffer_queue() {
        ...
        while (f54->is_busy) {
                mutex_unlock(&f54->data_mutex);
                if (!wait_for_completion_timeout(&f54->cmd_done, ...)) {
        ...
}

This executes synchronously while holding the V4L2 queue lock. Blocking here
via wait_for_completion_timeout() for up to 1000ms could prevent other users
from executing concurrent ioctls like DQBUF or STREAMOFF.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626051802.4033172-1-dmitry.torokhov@gmail.com?part=6