Re: [PATCH 10/10] Input: synaptics-rmi4 - use __le16 for FIFO offset in F54

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The struct f54_data is devm-allocated but embeds a struct video_device and sets its release callback to video_device_release_empty. This causes a Use-After-Free (UAF) upon driver unbind if userspace keeps the V4L2 file descriptor open.
--

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

Input: synaptics-rmi4 - use __le16 for FIFO offset in F54

Instead of manually splitting the 16-bit offset into a 2-byte array,
use __le16 and cpu_to_le16() to serialize the FIFO offset in
rmi_f54_work(). This is cleaner and more portable.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 5d045ece3c2f2..3eeb189be18b3 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c

[ ... ]

> @@ -597,12 +597,11 @@ static void rmi_f54_work(struct work_struct *work)
>  
>  	for (size_t i = 0; i < report_size; i += F54_REPORT_DATA_SIZE) {
>  		size_t size = min_t(size_t, F54_REPORT_DATA_SIZE, report_size - i);
> +		__le16 fifo = cpu_to_le16(i);
>  
> -		fifo[0] = i & 0xff;
> -		fifo[1] = i >> 8;
>  		error = rmi_write_block(fn->rmi_dev,
>  					fn->fd.data_base_addr + F54_FIFO_OFFSET,
> -					fifo, sizeof(fifo));
> +					&fifo, sizeof(fifo));

[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free
lifecycle bug related to how struct f54_data is managed.

The struct f54_data is devm-allocated in rmi_f54_probe():

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

However, it embeds a struct video_device and sets its release callback to
video_device_release_empty. If userspace keeps the V4L2 file descriptor open
during driver unbind, devres will immediately free f54_data when
rmi_f54_remove() completes:

    static void rmi_f54_remove(struct rmi_function *fn)
    {
            struct f54_data *f54 = dev_get_drvdata(&fn->dev);

            video_unregister_device(&f54->vdev);
            v4l2_device_unregister(&f54->v4l2);
            cancel_delayed_work_sync(&f54->work);
            destroy_workqueue(f54->workqueue);
    }

Since rmi_f54_remove() doesn't wait for the video device to be fully
released (or synchronize with in-flight V4L2 ioctls holding f54->lock),
an active ioctl could sleep and wake up after the struct is freed.

For example, if VIDIOC_QBUF blocks in rmi_f54_buffer_queue():

    if (!wait_for_completion_timeout(&f54->cmd_done,
                                     msecs_to_jiffies(1000))) {

Could this wake up after the driver is unbound and access the freed
f54->data_mutex or f54->sequence?

Does this violate the V4L2 lifecycle contract, which requires that memory 
backing a video_device must not be freed until the core invokes the .release 
callback?

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