Re: [PATCH 04/10] Input: synaptics-rmi4 - cancel delayed work on F54 remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `video_device` structure is embedded in a `devm`-allocated container with an empty release callback, leading to a Use-After-Free if the V4L2 device file is kept open after unbind.
- [High] Concurrent V4L2 ioctls are not synchronized or aborted during `rmi_f54_remove()`, leading to a Use-After-Free when an active ioctl times out or accesses a destroyed workqueue.
--

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

Input: synaptics-rmi4 - cancel delayed work on F54 remove

This commit ensures that pending delayed work is cancelled before destroying
the workqueue in rmi_f54_remove() to prevent a potential Use-After-Free. It
fixes a race condition where a timer handler might run after the workqueue
is destroyed or the devm-allocated f54 structure is freed.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -749,6 +749,7 @@ static void rmi_f54_remove(struct rmi_function *fn)
>  
>  	video_unregister_device(&f54->vdev);

[Severity: High]
This is a pre-existing issue, but the video_device structure is embedded in
a devm-allocated container with an empty release callback. This circumvents
the V4L2 core's mechanism to defer freeing the structure until all file
descriptors are closed:

drivers/input/rmi4/rmi_f54.c:rmi_f54_video_device {
    ...
    .release = video_device_release_empty,
};

In rmi_f54_probe():
    f54 = devm_kzalloc(&fn->dev, sizeof(struct f54_data), GFP_KERNEL);

If a userspace application holds the V4L2 device file descriptor open while
an unbind occurs, video_unregister_device() returns and devm immediately
frees f54_data. Closing the file descriptor later or issuing ioctls will
access the freed f54_data container. Could this lead to a Use-After-Free?

>  	v4l2_device_unregister(&f54->v4l2);
> +	cancel_delayed_work_sync(&f54->work);
>  	destroy_workqueue(f54->workqueue);

[Severity: High]
This isn't a bug introduced by this patch, but does this teardown sequence
destroy resources locklessly while V4L2 ioctls can still be executing?

During rmi_f54_remove(), we unregister the video device, cancel the work,
and destroy the workqueue without acquiring f54->lock.

If a concurrent VIDIOC_QBUF ioctl executing rmi_f54_buffer_queue() is
blocked:

drivers/input/rmi4/rmi_f54.c:rmi_f54_buffer_queue() {
    ...
    if (!wait_for_completion_timeout(&f54->cmd_done,
                                     msecs_to_jiffies(1000))) {
    ...
}

The remove() function cancels the delayed work (so cmd_done never completes)
and returns, allowing devm to free f54_data. When the timeout expires in the
blocked ioctl, it will access the freed f54 pointer.

Additionally, a concurrent ioctl might attempt to queue work:

drivers/input/rmi4/rmi_f54.c:rmi_f54_request_report() {
    ...
    queue_delayed_work(f54->workqueue, &f54->work, 0);
    ...
}

If this runs right after rmi_f54_remove() has destroyed the workqueue, could
it cause a crash? Should the teardown sequence serialize with active V4L2
ioctls?

>  }

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