[PATCH 01/10] Input: synaptics-rmi4 - fix F55 transmitter electrode count typo

[PATCH 01/10] Input: synaptics-rmi4 - fix F55 transmitter electrode count typo

[Dmitry Torokhov]

During F55 sensor detection, the transmitter (TX) electrode count was
incorrectly assigned the value of the receiver (RX) electrode count
due to copy-paste typos.

This incorrect value was then propagated to the driver data and used
by F54 to determine the diagnostics report size. On devices with more
RX than TX electrodes, this inflated the perceived TX count, leading
to incorrect report size calculations and potential out-of-bounds
buffer accesses.

Fix the typos by correctly assigning the TX electrode counts.

Fixes: 6adba43fd222 ("Input: synaptics-rmi4 - add support for F55 sensor tuning")
Fixes: c762cc68b6a1 ("Input: synaptics-rmi4 - propagate correct number of rx and tx electrodes to F54")
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f55.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
index 488adaca4dd0..a0877d32a914 100644
--- a/drivers/input/rmi4/rmi_f55.c
+++ b/drivers/input/rmi4/rmi_f55.c
@@ -54,10 +54,10 @@ static int rmi_f55_detect(struct rmi_function *fn)
 	f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
 
 	f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
-	f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
+	f55->cfg_num_tx_electrodes = f55->num_tx_electrodes;
 
 	drv_data->num_rx_electrodes = f55->cfg_num_rx_electrodes;
-	drv_data->num_tx_electrodes = f55->cfg_num_rx_electrodes;
+	drv_data->num_tx_electrodes = f55->cfg_num_tx_electrodes;
 
 	if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
 		int i, total;
-- 
2.55.0.rc0.799.gd6f94ed593-goog

[PATCH 02/10] Input: synaptics-rmi4 - zero report size on F54 work error

[Dmitry Torokhov]

In rmi_f54_work(), if an error occurs during report request or command
verification, the code jumped directly to the 'error' label, bypassing
the 'abort' label where f54->report_size was normally zeroed out.

This left f54->report_size containing its previous successful payload
size. If a user then altered the V4L2 format to a smaller size, and a
subsequent run failed, rmi_f54_buffer_queue() would copy the stale,
larger payload size into the shrunken V4L2 buffer, causing a heap
buffer overflow.

Fix this by merging the 'abort' and 'error' labels into a single 'out'
exit path, and ensuring that f54->report_size is always set to 0 on
failure by checking for error and zeroing the local report_size first.

Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54 diagnostics")
Cc: stable@vger.kernel.org
Reported-by: sashiko-bot@kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 61909e1a39e2..8eac320c43e3 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -545,7 +545,7 @@ static void rmi_f54_work(struct work_struct *work)
 		dev_err(&fn->dev, "Bad report size, report type=%d\n",
 				f54->report_type);
 		error = -EINVAL;
-		goto error;     /* retry won't help */
+		goto out;     /* retry won't help */
 	}
 
 	/*
@@ -556,7 +556,7 @@ static void rmi_f54_work(struct work_struct *work)
 			 &command);
 	if (error) {
 		dev_err(&fn->dev, "Failed to read back command\n");
-		goto error;
+		goto out;
 	}
 	if (command & F54_GET_REPORT) {
 		if (time_after(jiffies, f54->timeout)) {
@@ -564,7 +564,7 @@ static void rmi_f54_work(struct work_struct *work)
 			error = -ETIMEDOUT;
 		}
 		report_size = 0;
-		goto error;
+		goto out;
 	}
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "Get report command completed, reading data\n");
@@ -579,7 +579,7 @@ static void rmi_f54_work(struct work_struct *work)
 					fifo, sizeof(fifo));
 		if (error) {
 			dev_err(&fn->dev, "Failed to set fifo start offset\n");
-			goto abort;
+			goto out;
 		}
 
 		error = rmi_read_block(fn->rmi_dev, fn->fd.data_base_addr +
@@ -588,16 +588,16 @@ static void rmi_f54_work(struct work_struct *work)
 		if (error) {
 			dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n",
 				__func__, size, error);
-			goto abort;
+			goto out;
 		}
 	}
 
-abort:
-	f54->report_size = error ? 0 : report_size;
-error:
+out:
 	if (error)
 		report_size = 0;
 
+	f54->report_size = report_size;
+
 	if (report_size == 0 && !error) {
 		queue_delayed_work(f54->workqueue, &f54->work,
 				   msecs_to_jiffies(1));
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 02/10] Input: synaptics-rmi4 - zero report size on F54 work error

[sashiko-bot]

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

Pre-existing issues:
- [High] V4L2 structures are embedded in a `devm` allocated struct, leading to a Use-After-Free if a file descriptor is kept open after device unbind.
--

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

Input: synaptics-rmi4 - zero report size on F54 work error

This commit fixes a heap buffer overflow in the F54 diagnostic code. It
modifies the error path in rmi_f54_work() to ensure the report_size field is
always set to 0 on failure, preventing stale larger payload sizes from being
copied into shrunken v4l2 buffers.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 61909e1a39e24..8eac320c43e35 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
[ ... ]
> @@ -588,16 +588,16 @@ static void rmi_f54_work(struct work_struct *work)
>  		if (error) {
>  			dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n",
>  				__func__, size, error);
> -			goto abort;
> +			goto out;
>  		}
>  	}
>  
> -abort:
> -	f54->report_size = error ? 0 : report_size;
> -error:
> +out:
>  	if (error)
>  		report_size = 0;
>  
> +	f54->report_size = report_size;
> +

[Severity: High]
This is a pre-existing issue, but since we are modifying the v4l2-related f54
struct, is it safe to embed the video_device in a devm allocated struct?

Looking at rmi_f54_probe() and rmi_f54_remove():

drivers/input/rmi4/rmi_f54.c:rmi_f54_probe() {
    ...
    f54 = devm_kzalloc(&fn->dev, sizeof(struct f54_data), GFP_KERNEL);
    ...
    f54->vdev = rmi_f54_video_device;
    ...
    ret = video_register_device(&f54->vdev, VFL_TYPE_TOUCH, -1);
    ...
}

drivers/input/rmi4/rmi_f54.c:rmi_f54_remove() {
    ...
    video_unregister_device(&f54->vdev);
    ...
}

When rmi_f54_remove() completes, the driver core frees the devm allocated
memory for f54. If a user keeps /dev/videoX open during unbind, the
video_device refcount remains above zero, but its memory has been freed.

Would subsequent system calls (like ioctl) from the open fd attempt to
dereference vdev->fops from the freed f54 memory, resulting in a
use-after-free?

>  	if (report_size == 0 && !error) {
>  		queue_delayed_work(f54->workqueue, &f54->work,
>  				   msecs_to_jiffies(1));

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

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

[Dmitry Torokhov]

From: Bryam Vargas <hexlabsecurity@proton.me>

rmi_f54_work() reads a diagnostics report from the device into
f54->report_data, sizing the transfer with rmi_f54_get_report_size():

	report_size = rmi_f54_get_report_size(f54);
	...
	for (i = 0; i < report_size; i += F54_REPORT_DATA_SIZE) {
		int size = min(F54_REPORT_DATA_SIZE, report_size - i);
		...
		rmi_read_block(.., f54->report_data + i, size);
	}

report_data is allocated once at probe from F54's own electrode counts
(array3_size(f54->num_tx_electrodes, f54->num_rx_electrodes, sizeof(u16))),
but rmi_f54_get_report_size() computes the size from
drv_data->num_*_electrodes when those are set, i.e. from the F55
function's electrode counts. Both counts come straight from device
queries (F54 and F55 each report up to 255 electrodes) and nothing
constrains the F55 counts to the F54 ones.

A malicious or malfunctioning RMI4 device that reports larger F55
electrode counts than its F54 counts makes report_size exceed the
allocation, so the read loop writes past report_data (and the V4L2
dequeue memcpy() then reads past it). On conforming hardware the F55
configured electrodes are a subset of the F54 physical electrodes, so
report_size never exceeds the buffer and well-behaved devices are
unaffected.

Record the allocation size and reject a report that does not fit,
mirroring the existing zero-size check.

Fixes: c762cc68b6a1 ("Input: synaptics-rmi4 - propagate correct number of rx and tx electrodes to F54")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 8eac320c43e3..75839a54656b 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -104,6 +104,7 @@ struct f54_data {
 
 	enum rmi_f54_report_type report_type;
 	u8 *report_data;
+	size_t max_report_size;
 	int report_size;
 
 	bool is_busy;
@@ -548,6 +549,13 @@ static void rmi_f54_work(struct work_struct *work)
 		goto out;     /* retry won't help */
 	}
 
+	if (report_size > f54->max_report_size) {
+		dev_err(&fn->dev, "Report size %d exceeds buffer size %zu\n",
+			report_size, f54->max_report_size);
+		error = -EINVAL;
+		goto out;
+	}
+
 	/*
 	 * Need to check if command has completed.
 	 * If not try again later.
@@ -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->max_report_size = array3_size(tx, rx, sizeof(u16));
+	f54->report_data = devm_kzalloc(&fn->dev, f54->max_report_size,
 					GFP_KERNEL);
 	if (f54->report_data == NULL)
 		return -ENOMEM;
-- 
2.55.0.rc0.799.gd6f94ed593-goog

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Devm-managed V4L2 video_device leads to Use-After-Free on deferred file close.
- [Medium] Missing vb2_queue_release() leaks videobuf2 resources during device unbind.
--

commit 7c7a1b6617a82d10a8cfe2c055e62e4a1c8fe287
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer

This commit prevents a buffer overflow in rmi_f54_work() by rejecting reports
that are larger than the allocated size of f54->report_data. It records the
allocation size during probe and ensures the report_size does not exceed it.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 8eac320c43e35..75839a54656b9 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -104,6 +104,7 @@ struct f54_data {
>  
>  	enum rmi_f54_report_type report_type;
>  	u8 *report_data;
> +	size_t max_report_size;
>  	int report_size;
>  
>  	bool is_busy;

[Severity: High]
This isn't a bug introduced by this patch, but looking at struct f54_data
which embeds struct video_device vdev, can its devm-managed lifecycle lead
to a use-after-free on a deferred file close?

If a user holds the video node open while the device is unbound,
rmi_f54_remove() calls video_unregister_device(&f54->vdev).

Devres will then free struct f54_data immediately upon return.

When the user later closes the file descriptor, will the V4L2 core access
the freed video_device to decrement its kref and invoke the release callback?


[Severity: Medium]
This is a pre-existing issue, but does rmi_f54_remove() leak videobuf2
resources during device unbind?

The teardown path unregisters the video device with
video_unregister_device(&f54->vdev).

If userspace requests buffers (e.g., via VIDIOC_REQBUFS) and the device is
subsequently unbound without the application cleanly releasing the buffers,
will the driver leak the videobuf2 kernel resources and allocated buffers
since it fails to call vb2_queue_release()?

[ ... ]

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

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

[Dmitry Torokhov]

Ensure that any pending delayed work is cancelled before destroying
the workqueue in rmi_f54_remove() to prevent a potential Use-After-Free.

While destroy_workqueue() drains the queue, it does not cancel pending
timers for delayed work. If the timer has not yet expired when
destroy_workqueue() is called, the work is not in the queue yet. Once
the timer expires later, the timer handler will attempt to queue the
work onto the already destroyed workqueue, or access the freed f54
structure (since it is devm-allocated), leading to a crash.

Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54 diagnostics")
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 75839a54656b..aebe74d2032c 100644
--- 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);
 	v4l2_device_unregister(&f54->v4l2);
+	cancel_delayed_work_sync(&f54->work);
 	destroy_workqueue(f54->workqueue);
 }
 
-- 
2.55.0.rc0.799.gd6f94ed593-goog

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

[sashiko-bot]

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

[PATCH 05/10] Input: synaptics-rmi4 - block s_input when F54 queue is busy

[Dmitry Torokhov]

Changing the input (diagnostic report type) mid-stream changes the
report size. Since V4L2 buffers are allocated based on the size at
stream start, changing the input while streaming could lead to a
heap buffer overflow if the new size is larger than the allocated
buffers.

Prevent this by blocking VIDIOC_S_INPUT with -EBUSY if the V4L2 queue
is busy (streaming).

Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54 diagnostics")
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index aebe74d2032c..e86dfc9ce7d9 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -445,7 +445,12 @@ static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
 
 static int rmi_f54_vidioc_s_input(struct file *file, void *priv, unsigned int i)
 {
-	return rmi_f54_set_input(video_drvdata(file), i);
+	struct f54_data *f54 = video_drvdata(file);
+
+	if (vb2_is_busy(&f54->queue))
+		return -EBUSY;
+
+	return rmi_f54_set_input(f54, i);
 }
 
 static int rmi_f54_vidioc_g_input(struct file *file, void *priv,
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 05/10] Input: synaptics-rmi4 - block s_input when F54 queue is busy

[sashiko-bot]

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

Pre-existing issues:
- [High] Using `devm_kzalloc` for a structure containing a V4L2 `video_device` with `video_device_release_empty` leads to a Use-After-Free on unbind.
--

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

Input: synaptics-rmi4 - block s_input when F54 queue is busy

Changing the input (diagnostic report type) mid-stream changes the report size.
Since V4L2 buffers are allocated based on the size at stream start, changing
the input while streaming could lead to a heap buffer overflow if the new size
is larger than the allocated buffers.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index aebe74d2032cd..e86dfc9ce7d9a 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -445,7 +445,12 @@ static int rmi_f54_set_input(struct f54_data *f54, unsigned int i)
>  
>  static int rmi_f54_vidioc_s_input(struct file *file, void *priv, unsigned int i)
>  {
> -	return rmi_f54_set_input(video_drvdata(file), i);
> +	struct f54_data *f54 = video_drvdata(file);

[Severity: High]
This isn't a bug introduced by this patch, but could this f54 pointer
reference freed memory during a device unbind?

In rmi_f54_probe(), the f54_data structure is allocated with devm_kzalloc():

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

This ties its lifetime to the physical device rather than the V4L2 character
device reference count. The release callback for the video device is also set
to a dummy function in rmi_f54_video_device:

    .release = video_device_release_empty,

During unbind, rmi_f54_remove() calls video_unregister_device(&f54->vdev) but
returns immediately. This unregisters the V4L2 device without synchronously
waiting for userspace to close open file descriptors, leaving ongoing V4L2
file descriptors pointing at memory that is about to be freed by devres.

If a userspace application keeps the V4L2 device node open, would subsequent
operations or closes (such as vb2_fop_release) attempt to access the vdev and
queue embedded within this already-freed f54_data memory?

> +
> +	if (vb2_is_busy(&f54->queue))
> +		return -EBUSY;
> +
> +	return rmi_f54_set_input(f54, i);
>  }

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

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

[Dmitry Torokhov]

Previously, rmi_f54_buffer_queue() waited for the worker thread to
finish but ignored whether it succeeded. If the worker failed (e.g.,
due to a timeout or register read failure), the queue thread would
silently return success, delivering stale or uninitialized memory to
userspace.

Add a 'report_error' field to struct f54_data to store the worker's exit
status. Check this field in rmi_f54_buffer_queue() after the worker
finishes, and mark the buffer as VB2_BUF_STATE_ERROR if an error
occurred.

Fixes: 3a762dbd5347 ("Input: synaptics-rmi4 - add support for F54 diagnostics")
Reported-by: sashiko-bot@kernel.org
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index e86dfc9ce7d9..c86bc81845bb 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;
 
 	bool is_busy;
 	struct mutex status_mutex;
@@ -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;
+	}
+
 	ptr = vb2_plane_vaddr(vb, 0);
 	if (!ptr) {
 		dev_err(&f54->fn->dev, "Error acquiring frame ptr\n");
@@ -610,6 +617,7 @@ static void rmi_f54_work(struct work_struct *work)
 		report_size = 0;
 
 	f54->report_size = report_size;
+	f54->report_error = error;
 
 	if (report_size == 0 && !error) {
 		queue_delayed_work(f54->workqueue, &f54->work,
-- 
2.55.0.rc0.799.gd6f94ed593-goog

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

[sashiko-bot]

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

[PATCH 07/10] Input: synaptics-rmi4 - check V4L2 buffer size in F54 queue

[Dmitry Torokhov]

Add a safety check in rmi_f54_buffer_queue() to ensure that the
requested report size (f54->report_size) does not exceed the actual
allocated size of the V4L2 buffer (vb2_plane_size()).

This provides a defense-in-depth measure against any potential size
mismatches between the V4L2 queue and the driver's internal state.

Fixes: 3a762dbd5347 ("[media] Input: synaptics-rmi4 - add support for F54 diagnostics")
Cc: stable@vger.kernel.org
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index c86bc81845bb..93526feea563 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -354,6 +354,13 @@ static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
 		goto data_done;
 	}
 
+	if (f54->report_size > vb2_plane_size(vb, 0)) {
+		dev_err(&f54->fn->dev, "Buffer too small (%lu < %d)\n",
+			vb2_plane_size(vb, 0), f54->report_size);
+		state = VB2_BUF_STATE_ERROR;
+		goto data_done;
+	}
+
 	memcpy(ptr, f54->report_data, f54->report_size);
 	vb2_set_plane_payload(vb, 0, rmi_f54_get_report_size(f54));
 	state = VB2_BUF_STATE_DONE;
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 07/10] Input: synaptics-rmi4 - check V4L2 buffer size in F54 queue

[sashiko-bot]

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

Pre-existing issues:
- [Critical] The driver embeds V4L2 structures (`video_device`, `vb2_queue`, `mutex`) within `struct f54_data` allocated using `devm_kzalloc()`, leading to a use-after-free when the device is unbound while file descriptors remain open.
--

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

Input: synaptics-rmi4 - check V4L2 buffer size in F54 queue

Add a safety check in rmi_f54_buffer_queue() to ensure that the requested
report size (f54->report_size) does not exceed the actual allocated size of
the V4L2 buffer (vb2_plane_size()). This provides a defense-in-depth
measure against any potential size mismatches between the V4L2 queue and
the driver's internal state.

[Severity: Critical]
This is a pre-existing issue, but does the driver have a use-after-free
in the V4L2 device unbind path?

In drivers/input/rmi4/rmi_f54.c:rmi_f54_probe(), f54_data is allocated
using devm:

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

And in rmi_f54_remove():

    video_unregister_device(&f54->vdev);

When the device is unbound while a user holds the V4L2 file descriptor
open, devres will immediately free f54_data after rmi_f54_remove()
returns. However, the V4L2 core retains a reference to the embedded
video_device for the open file descriptor.

Additionally, vb2_queue_release() is never called, leaving active V4L2
waitqueues pointing to the freed memory.

Can subsequent operations on the open file descriptor dereference the
freed f54->vdev, f54->lock, or f54->queue?

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index c86bc81845bb1..93526feea5636 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
[ ... ]

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

[PATCH 08/10] Input: synaptics-rmi4 - F54 style and typo fixes

[Dmitry Torokhov]

Clean up style issues in rmi_f54.c reported by checkpatch.pl --strict:
- Convert (1 << X) caps to BIT(X).
- Align assignment operator '=' in report_types array.
- Add comments to status_mutex, data_mutex, and lock explaining what they protect.
- Align function arguments/parameters with open parenthesis in rmi_f54_get_reptype() and rmi_f54_vidioc_querycap().
- Fix typo 'firmare' -> 'firmware' in comment.
- Align dev_err() argument with open parenthesis in rmi_f54_work().
- Use '!ptr' instead of 'ptr == NULL' for report_data check in rmi_f54_probe().

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 38 ++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 93526feea563..850e1742c480 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -5,6 +5,7 @@
  */
 
 #include <linux/kernel.h>
+#include <linux/bits.h>
 #include <linux/rmi.h>
 #include <linux/input.h>
 #include <linux/slab.h>
@@ -35,9 +36,9 @@
 #define F54_FORCE_CAL           2
 
 /* F54 capabilities */
-#define F54_CAP_BASELINE	(1 << 2)
-#define F54_CAP_IMAGE8		(1 << 3)
-#define F54_CAP_IMAGE16		(1 << 6)
+#define F54_CAP_BASELINE	BIT(2)
+#define F54_CAP_IMAGE8		BIT(3)
+#define F54_CAP_IMAGE16		BIT(6)
 
 /**
  * enum rmi_f54_report_type - RMI4 F54 report types
@@ -83,14 +84,13 @@ enum rmi_f54_report_type {
 };
 
 static const char * const rmi_f54_report_type_names[] = {
-	[F54_REPORT_NONE]		= "Unknown",
-	[F54_8BIT_IMAGE]		= "Normalized 8-Bit Image",
-	[F54_16BIT_IMAGE]		= "Normalized 16-Bit Image",
-	[F54_RAW_16BIT_IMAGE]		= "Raw 16-Bit Image",
-	[F54_TRUE_BASELINE]		= "True Baseline",
-	[F54_FULL_RAW_CAP]		= "Full Raw Capacitance",
-	[F54_FULL_RAW_CAP_RX_OFFSET_REMOVED]
-					= "Full Raw Capacitance RX Offset Removed",
+	[F54_REPORT_NONE]			= "Unknown",
+	[F54_8BIT_IMAGE]			= "Normalized 8-Bit Image",
+	[F54_16BIT_IMAGE]			= "Normalized 16-Bit Image",
+	[F54_RAW_16BIT_IMAGE]			= "Raw 16-Bit Image",
+	[F54_TRUE_BASELINE]			= "True Baseline",
+	[F54_FULL_RAW_CAP]			= "Full Raw Capacitance",
+	[F54_FULL_RAW_CAP_RX_OFFSET_REMOVED]	= "Full Raw Capacitance RX Offset Removed",
 };
 
 struct f54_data {
@@ -109,8 +109,8 @@ struct f54_data {
 	int report_error;
 
 	bool is_busy;
-	struct mutex status_mutex;
-	struct mutex data_mutex;
+	struct mutex status_mutex; /* Protects is_busy and command state */
+	struct mutex data_mutex;   /* Protects report_data buffer */
 
 	struct workqueue_struct *workqueue;
 	struct delayed_work work;
@@ -123,7 +123,7 @@ struct f54_data {
 	struct v4l2_pix_format format;
 	struct video_device vdev;
 	struct vb2_queue queue;
-	struct mutex lock;
+	struct mutex lock; /* Serializes V4L2 device and queue access */
 	u32 sequence;
 	int input;
 	enum rmi_f54_report_type inputs[F54_MAX_REPORT_TYPE];
@@ -153,7 +153,7 @@ static bool is_f54_report_type_valid(struct f54_data *f54,
 }
 
 static enum rmi_f54_report_type rmi_f54_get_reptype(struct f54_data *f54,
-						unsigned int i)
+						    unsigned int i)
 {
 	if (i >= F54_MAX_REPORT_TYPE)
 		return F54_REPORT_NONE;
@@ -193,7 +193,7 @@ static int rmi_f54_request_report(struct rmi_function *fn, u8 report_type)
 
 	/*
 	 * Small delay after disabling interrupts to avoid race condition
-	 * in firmare. This value is a bit higher than absolutely necessary.
+	 * in firmware. This value is a bit higher than absolutely necessary.
 	 * Should be removed once issue is resolved in firmware.
 	 */
 	usleep_range(2000, 3000);
@@ -406,7 +406,7 @@ static int rmi_f54_vidioc_querycap(struct file *file, void *priv,
 	strscpy(cap->driver, F54_NAME, sizeof(cap->driver));
 	strscpy(cap->card, SYNAPTICS_INPUT_DEVICE_NAME, sizeof(cap->card));
 	snprintf(cap->bus_info, sizeof(cap->bus_info),
-		"rmi4:%s", dev_name(&f54->fn->dev));
+		 "rmi4:%s", dev_name(&f54->fn->dev));
 
 	return 0;
 }
@@ -563,7 +563,7 @@ static void rmi_f54_work(struct work_struct *work)
 	report_size = rmi_f54_get_report_size(f54);
 	if (report_size == 0) {
 		dev_err(&fn->dev, "Bad report size, report type=%d\n",
-				f54->report_type);
+			f54->report_type);
 		error = -EINVAL;
 		goto out;     /* retry won't help */
 	}
@@ -709,7 +709,7 @@ static int rmi_f54_probe(struct rmi_function *fn)
 	f54->max_report_size = array3_size(tx, rx, sizeof(u16));
 	f54->report_data = devm_kzalloc(&fn->dev, f54->max_report_size,
 					GFP_KERNEL);
-	if (f54->report_data == NULL)
+	if (!f54->report_data)
 		return -ENOMEM;
 
 	INIT_DELAYED_WORK(&f54->work, rmi_f54_work);
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 08/10] Input: synaptics-rmi4 - F54 style and typo fixes

[sashiko-bot]

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

New issues:
- [Low] The newly added comments incorrectly describe the lock coverage for `status_mutex` and `data_mutex`.

Pre-existing issues:
- [High] Embedding a `struct video_device` in a `devm_kzalloc`-managed structure causes a use-after-free on device unbind.
--

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

Input: synaptics-rmi4 - F54 style and typo fixes

This commit cleans up style issues in rmi_f54.c reported by checkpatch.pl.
It converts bit shifts to BIT() macros, aligns assignments and arguments,
fixes typos, and adds mutex documentation comments.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 93526feea5636..850e1742c480a 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
[ ... ]
> @@ -109,8 +109,8 @@ struct f54_data {
>  	int report_error;
>  
>  	bool is_busy;
> -	struct mutex status_mutex;
> -	struct mutex data_mutex;
> +	struct mutex status_mutex; /* Protects is_busy and command state */
> +	struct mutex data_mutex;   /* Protects report_data buffer */

[Severity: Low]
Are these comments accurate regarding the lock coverage?

Looking at rmi_f54_request_report() and rmi_f54_work(), it appears that
is_busy, cmd_done, and timeout are explicitly modified while holding
data_mutex, not status_mutex.

status_mutex appears to be used to serialize calls to rmi_f54_buffer_queue()
instead.

[ ... ]
> @@ -709,7 +709,7 @@ static int rmi_f54_probe(struct rmi_function *fn)
>  	f54->max_report_size = array3_size(tx, rx, sizeof(u16));
>  	f54->report_data = devm_kzalloc(&fn->dev, f54->max_report_size,
>  					GFP_KERNEL);
> -	if (f54->report_data == NULL)
> +	if (!f54->report_data)
>  		return -ENOMEM;
>  
>  	INIT_DELAYED_WORK(&f54->work, rmi_f54_work);

[Severity: High]
This isn't a bug introduced by this patch, but does allocating the f54_data
structure with devm_kzalloc() earlier in rmi_f54_probe() lead to a
use-after-free on device unbind?

If a userspace process opens the v4l2 device node, and concurrently the
physical device is hot-unplugged or unbound via sysfs, devm will
automatically free the f54_data memory.

However, since the release callback for the embedded struct video_device is
set to video_device_release_empty, the v4l2 core retains a reference to the
embedded video_device.

Could it then access the already-freed memory during ioctls or when the last
file descriptor is closed?

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

[PATCH 09/10] Input: synaptics-rmi4 - change report_size to size_t in F54

[Dmitry Torokhov]

Change report_size from int to size_t in struct f54_data and update all
its usages. This avoids signed/unsigned comparison warnings when
comparing with size_t variables and matches the signature of
rmi_read_block() which takes size_t for length.

Also move the declaration of the loop variable 'i' into the 'for' loop
header.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 850e1742c480..5d045ece3c2f 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -105,7 +105,7 @@ struct f54_data {
 	enum rmi_f54_report_type report_type;
 	u8 *report_data;
 	size_t max_report_size;
-	int report_size;
+	size_t report_size;
 	int report_error;
 
 	bool is_busy;
@@ -355,7 +355,7 @@ static void rmi_f54_buffer_queue(struct vb2_buffer *vb)
 	}
 
 	if (f54->report_size > vb2_plane_size(vb, 0)) {
-		dev_err(&f54->fn->dev, "Buffer too small (%lu < %d)\n",
+		dev_err(&f54->fn->dev, "Buffer too small (%lu < %zu)\n",
 			vb2_plane_size(vb, 0), f54->report_size);
 		state = VB2_BUF_STATE_ERROR;
 		goto data_done;
@@ -553,10 +553,9 @@ static void rmi_f54_work(struct work_struct *work)
 	struct f54_data *f54 = container_of(work, struct f54_data, work.work);
 	struct rmi_function *fn = f54->fn;
 	u8 fifo[2];
-	int report_size;
+	size_t report_size;
 	u8 command;
 	int error;
-	int i;
 
 	mutex_lock(&f54->data_mutex);
 
@@ -569,7 +568,7 @@ static void rmi_f54_work(struct work_struct *work)
 	}
 
 	if (report_size > f54->max_report_size) {
-		dev_err(&fn->dev, "Report size %d exceeds buffer size %zu\n",
+		dev_err(&fn->dev, "Report size %zu exceeds buffer size %zu\n",
 			report_size, f54->max_report_size);
 		error = -EINVAL;
 		goto out;
@@ -596,8 +595,8 @@ static void rmi_f54_work(struct work_struct *work)
 
 	rmi_dbg(RMI_DEBUG_FN, &fn->dev, "Get report command completed, reading data\n");
 
-	for (i = 0; i < report_size; i += F54_REPORT_DATA_SIZE) {
-		int size = min(F54_REPORT_DATA_SIZE, report_size - i);
+	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);
 
 		fifo[0] = i & 0xff;
 		fifo[1] = i >> 8;
@@ -613,7 +612,7 @@ static void rmi_f54_work(struct work_struct *work)
 				       F54_REPORT_DATA_OFFSET,
 				       f54->report_data + i, size);
 		if (error) {
-			dev_err(&fn->dev, "%s: read [%d bytes] returned %d\n",
+			dev_err(&fn->dev, "%s: read [%zu bytes] returned %d\n",
 				__func__, size, error);
 			goto out;
 		}
-- 
2.55.0.rc0.799.gd6f94ed593-goog

Re: [PATCH 09/10] Input: synaptics-rmi4 - change report_size to size_t in F54

[sashiko-bot]

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

Pre-existing issues:
- [High] The v4l2 device structure (vdev, queue, and lock) is embedded within devm-allocated f54_data, causing a use-after-free and locking of a freed mutex on device close if userspace holds the device open past removal.
--

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

Input: synaptics-rmi4 - change report_size to size_t in F54

Change report_size from int to size_t in struct f54_data and update all
its usages. This avoids signed/unsigned comparison warnings when
comparing with size_t variables and matches the signature of
rmi_read_block() which takes size_t for length.

> diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
> index 850e1742c480a..5d045ece3c2f2 100644
> --- a/drivers/input/rmi4/rmi_f54.c
> +++ b/drivers/input/rmi4/rmi_f54.c
> @@ -105,7 +105,7 @@ struct f54_data {
>  	enum rmi_f54_report_type report_type;
>  	u8 *report_data;
>  	size_t max_report_size;
> -	int report_size;
> +	size_t report_size;
>  	int report_error;
>  
>  	bool is_busy;

[Severity: High]
This is a pre-existing issue, but could embedding the V4L2 device structures
inside the devm-allocated f54_data lead to a use-after-free?

If a user holds the /dev/videoX node open during driver unbind,
rmi_f54_remove() completes and the devm framework frees the f54_data memory:

drivers/input/rmi4/rmi_f54.c:rmi_f54_remove() {
    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);
}

When the user later closes the file descriptor, it triggers the V4L2
release path. v4l2_release() calling vb2_fop_release() accesses the
embedded queue and attempts to acquire the vdev->queue->lock, which
points to the freed f54->lock:

drivers/input/rmi4/rmi_f54.c:rmi_f54_probe() {
    ...
    f54 = devm_kzalloc(&fn->dev, sizeof(struct f54_data), GFP_KERNEL);
    ...
    f54->vdev = rmi_f54_video_device;
    f54->vdev.v4l2_dev = &f54->v4l2;
    f54->vdev.lock = &f54->lock;
    ...
}

Should the video_device be dynamically allocated and reference counted rather
than relying on devm management?

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

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

[Dmitry Torokhov]

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.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/rmi4/rmi_f54.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/input/rmi4/rmi_f54.c b/drivers/input/rmi4/rmi_f54.c
index 5d045ece3c2f..3eeb189be18b 100644
--- a/drivers/input/rmi4/rmi_f54.c
+++ b/drivers/input/rmi4/rmi_f54.c
@@ -15,6 +15,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/videobuf2-v4l2.h>
 #include <media/videobuf2-vmalloc.h>
+#include <asm/byteorder.h>
 #include "rmi_driver.h"
 
 #define F54_NAME		"rmi4_f54"
@@ -552,7 +553,6 @@ static void rmi_f54_work(struct work_struct *work)
 {
 	struct f54_data *f54 = container_of(work, struct f54_data, work.work);
 	struct rmi_function *fn = f54->fn;
-	u8 fifo[2];
 	size_t report_size;
 	u8 command;
 	int error;
@@ -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));
 		if (error) {
 			dev_err(&fn->dev, "Failed to set fifo start offset\n");
 			goto out;
-- 
2.55.0.rc0.799.gd6f94ed593-goog

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

[sashiko-bot]

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

Re: [PATCH 01/10] Input: synaptics-rmi4 - fix F55 transmitter electrode count typo

[sashiko-bot]

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

Pre-existing issues:
- [Medium] F55:Ctrl2 register array is read from an incorrect static offset, causing overlapping reads with the preceding F55:Ctrl1 register array.
--

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

    Input: synaptics-rmi4 - fix F55 transmitter electrode count typo

    This commit fixes a typo during F55 sensor detection where the transmitter
    electrode count was incorrectly assigned the value of the receiver electrode
    count. The incorrect value was then propagated to the driver data and
    inflated the perceived TX count for F54, potentially leading to
    out-of-bounds buffer accesses.

> diff --git a/drivers/input/rmi4/rmi_f55.c b/drivers/input/rmi4/rmi_f55.c
> index 488adaca4dd00..a0877d32a9141 100644
> --- a/drivers/input/rmi4/rmi_f55.c
> +++ b/drivers/input/rmi4/rmi_f55.c
> @@ -54,10 +54,10 @@ static int rmi_f55_detect(struct rmi_function *fn)
>  	f55->num_tx_electrodes = f55->qry[F55_NUM_TX_OFFSET];
>  
>  	f55->cfg_num_rx_electrodes = f55->num_rx_electrodes;
> -	f55->cfg_num_tx_electrodes = f55->num_rx_electrodes;
> +	f55->cfg_num_tx_electrodes = f55->num_tx_electrodes;
>  
>  	drv_data->num_rx_electrodes = f55->cfg_num_rx_electrodes;
> -	drv_data->num_tx_electrodes = f55->cfg_num_rx_electrodes;
> +	drv_data->num_tx_electrodes = f55->cfg_num_tx_electrodes;
>  
>  	if (f55->qry[F55_PHYS_CHAR_OFFSET] & F55_CAP_SENSOR_ASSIGN) {
>  		int i, total;

[Severity: Medium]
This is a pre-existing issue, but I have a question about the offset used when
reading the F55:Ctrl2 register array.

Just below this hunk in rmi_f55_detect(), the code reads the transmitter
assignment array using a hardcoded +2 offset:

drivers/input/rmi4/rmi_f55.c:rmi_f55_detect() {
    ...
		error = rmi_read_block(fn->rmi_dev,
				       fn->fd.control_base_addr + 1,
				       buf, f55->num_rx_electrodes);
    ...
		error = rmi_read_block(fn->rmi_dev,
				       fn->fd.control_base_addr + 2,
				       buf, f55->num_tx_electrodes);
    ...
}

Will using a hardcoded +2 offset for Ctrl2 cause it to overlap with the Ctrl1
array if num_rx_electrodes > 1, or skip a byte if num_rx_electrodes == 0?

Should the offset dynamically account for the size of Ctrl1, such as using
control_base_addr + 1 + f55->num_rx_electrodes?

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