* Re: [PATCH 07/10] Input: synaptics-rmi4 - check V4L2 buffer size in F54 queue
From: sashiko-bot @ 2026-06-26 5:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260626051802.4033172-7-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 03/10] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer
From: sashiko-bot @ 2026-06-26 5:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260626051802.4033172-3-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 01/10] Input: synaptics-rmi4 - fix F55 transmitter electrode count typo
From: sashiko-bot @ 2026-06-26 5:31 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 09/10] Input: synaptics-rmi4 - change report_size to size_t in F54
From: sashiko-bot @ 2026-06-26 5:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260626051802.4033172-9-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 08/10] Input: synaptics-rmi4 - F54 style and typo fixes
From: sashiko-bot @ 2026-06-26 5:29 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260626051802.4033172-8-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 04/10] Input: synaptics-rmi4 - cancel delayed work on F54 remove
From: sashiko-bot @ 2026-06-26 5:28 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260626051802.4033172-4-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH 05/10] Input: synaptics-rmi4 - block s_input when F54 queue is busy
From: sashiko-bot @ 2026-06-26 5:27 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260626051802.4033172-5-dmitry.torokhov@gmail.com>
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
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer
From: Dmitry Torokhov @ 2026-06-26 5:19 UTC (permalink / raw)
To: hexlabsecurity
Cc: linux-input, linux-kernel, Guenter Roeck, Benjamin Tissoires
In-Reply-To: <20260613-b4-disp-6721686c-v1-1-4d5bb84ee520@proton.me>
Hi Bryam,
On Sat, Jun 13, 2026 at 11:01:16PM -0500, Bryam Vargas via B4 Relay wrote:
> 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>
Thanks for the patch. It makes sense, but there are more changes needed
in F54. I incorporated it in the series I just posted, would appreviate
if you could review it.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH 10/10] Input: synaptics-rmi4 - use __le16 for FIFO offset in F54
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 09/10] Input: synaptics-rmi4 - change report_size to size_t in F54
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 08/10] Input: synaptics-rmi4 - F54 style and typo fixes
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 07/10] Input: synaptics-rmi4 - check V4L2 buffer size in F54 queue
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, stable
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 06/10] Input: synaptics-rmi4 - propagate F54 worker errors to V4L2 queue
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, sashiko-bot, stable
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 05/10] Input: synaptics-rmi4 - block s_input when F54 queue is busy
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, stable
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 04/10] Input: synaptics-rmi4 - cancel delayed work on F54 remove
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, sashiko-bot, stable
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 03/10] Input: synaptics-rmi4 - bound the F54 report size to the allocated buffer
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, stable
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 02/10] Input: synaptics-rmi4 - zero report size on F54 work error
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, stable, sashiko-bot
In-Reply-To: <20260626051802.4033172-1-dmitry.torokhov@gmail.com>
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
^ permalink raw reply related
* [PATCH 01/10] Input: synaptics-rmi4 - fix F55 transmitter electrode count typo
From: Dmitry Torokhov @ 2026-06-26 5:17 UTC (permalink / raw)
To: Bryam Vargas, Hans Verkuil; +Cc: linux-input, linux-kernel, sashiko-bot, stable
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
^ permalink raw reply related
* Re: [PATCH v6 3/3] HID: nintendo: Add unified report format support
From: Vicki Pfau @ 2026-06-26 2:23 UTC (permalink / raw)
To: sashiko-reviews; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260625051335.6F41E1F000E9@smtp.kernel.org>
On 6/24/26 10:13 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Global minimum report size check incorrectly rejects valid legacy, non-unified reports.
> --
>
> commit ebbc8f2a738bc80390ec8c8785093f44578537e8
> Author: Vicki Pfau <vi@endrift.com>
>
> HID: nintendo: Add unified report format support
>
> This commit adds support for the unified report format that all controllers
> also support. It includes overlapping fields for like buttons and axes between
> them.
>
>> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
>> index ae360f4d15d1a..9cb4a503fe2d3 100644
>> --- a/drivers/hid/hid-nintendo.c
>> +++ b/drivers/hid/hid-nintendo.c
>
> [ ... ]
>
>> @@ -3732,7 +3833,7 @@ static int switch2_event(struct hid_device *hdev, struct hid_report *report, uin
>> if (report->type != HID_INPUT_REPORT)
>> return 0;
>>
>> - if (size < 15)
>> + if (size < 17)
>> return -EINVAL;
>>
>
> [Severity: High]
> Does this global size check incorrectly reject valid non-unified reports?
>
> Since non-unified reports like NS2_REPORT_GC only access up to raw_data[14]
> and can legitimately have a size of exactly 15 bytes, it seems this check will
> cause those legitimate 15-byte or 16-byte reports to be dropped with -EINVAL
> in switch2_event().
>
> Should this 17-byte requirement be scoped inside the case NS2_REPORT_UNIFIED:
> block instead?
>
Generally speaking, these reports should always be 63 bytes. This check is mostly here as a basic sanity check to make sure we don't accidentally reach out of bounds in the case of something very weird happening. It's fine if it seems a little overzealous since it's so far beyond normal operation already. I would guess than the Switch 2 rejects anything that short out-of-hand already.
^ permalink raw reply
* Re: [PATCH 0/2] (no cover subject)
From: Dmitry Torokhov @ 2026-06-26 0:47 UTC (permalink / raw)
To: hexlabsecurity
Cc: linux-kernel, Hans de Goede, Vincent Huang, linux-input,
Andrew Duggan
In-Reply-To: <20260614-b4-disp-818d6bda-v1-0-cf39a3615085@proton.me>
On Sun, Jun 14, 2026 at 12:36:10AM -0500, Bryam Vargas via B4 Relay wrote:
> From be2c4843f6145374f28edd25cef43c9373542874 Mon Sep 17 00:00:00 2001
> Message-ID: <cover.1781415009.git.hexlabsecurity@proton.me>
> From: Bryam Vargas <hexlabsecurity@proton.me>
> Date: Sun, 14 Jun 2026 00:30:09 -0500
> Subject: [PATCH 0/2] Input: synaptics-rmi4 - fix out-of-bounds keymap access on large GPIO counts
>
> The RMI4 F3A and F30 function handlers size their GPIO/LED key map to
> min(device_gpio_count, TRACKSTICK_RANGE_END) == at most 6 entries, but
> then consume that map with the full, unclamped device-reported count in
> two places: the attention-interrupt report loop, and input->keycodemax
> (while input->keycode points at the small allocation).
>
> A device that reports a GPIO/LED count greater than 6 therefore yields a
> key map of at most 12 bytes that is indexed up to count-1:
>
> - rmi_f3a_attention() / rmi_f30_attention() read gpio[led]_key_map[i]
> out of bounds on every attention interrupt (device-triggered), and
>
> - the input core's default keymap ioctls bound the index only against
> keycodemax, so EVIOCGKEYCODE reads adjacent slab memory back to user
> space and EVIOCSKEYCODE writes a caller-controlled u16 past the
> allocation -- a controlled out-of-bounds heap write reachable by any
> process that can open the evdev node.
>
> Both handlers have the same shape; F3A was copied from F30. Patch 1 fixes
> F3A, patch 2 fixes F30. The one-line fix in each sizes the key map to the
> full device count; the mapping loop is unchanged (it still populates only
> the first min(count, 6) entries, the rest stay KEY_RESERVED and are
> skipped on report), and keycodemax now matches the allocation, closing
> the interrupt and both ioctl paths.
>
> Reproduced with KASAN on a faithful in-kernel model of each handler
> (kmalloc-16 bucket, 12-byte request, reported count = 127):
>
> # without the patch -- attention read path
> BUG: KASAN: slab-out-of-bounds in rmi_f3a_attention
> Read of size 2 ... 0 bytes to the right of allocated 12-byte region
> ... cache kmalloc-16
>
> # without the patch -- EVIOCSKEYCODE write path
> BUG: KASAN: slab-out-of-bounds ...
> Write of size 2 ... 0 bytes to the right of allocated 12-byte region
>
> # with the patch (key map sized to the full count): clean
> # benign device (reported count <= 6): clean
>
> The full read and write splats and a 32/64-bit ASan model of the same
> geometry are available on request.
Applied the lot, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v6 2/2] Input: isa1200 - new driver for Imagis ISA1200
From: Dmitry Torokhov @ 2026-06-25 23:45 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Linus Walleij,
linux-input, devicetree, linux-kernel
In-Reply-To: <20260617070528.35006-3-clamor95@gmail.com>
Hi Svyatoslav,
On Wed, Jun 17, 2026 at 10:05:27AM +0300, Svyatoslav Ryhel wrote:
> From: Linus Walleij <linusw@kernel.org>
>
> The ISA1200 is a haptic feedback unit from Imagis Technology using two
> motors for haptic feedback in mobile phones. Used in many mobile devices
> c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
> GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.
>
> The exact datasheet for the ISA1200 is not available; all data was modeled
> based on available downstream kernel sources for various devices and
> fragments of information scattered across the internet.
>
> Tested-by: Linus Walleij <linusw@kernel.org> # GT-I9070 Janice
> Signed-off-by: Linus Walleij <linusw@kernel.org>
> Co-developed-by: Svyatoslav Ryhel <clamor95@gmail.com>
> Signed-off-by: Svyatoslav Ryhel <clamor95@gmail.com>
Sashiko convinced me that using mutex_trylock() in the playback work
handler will result in dropping requests and that we are safe not taking
the lock there at all. Can you please try the following modification?
diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
index c61adc4b605c..fb7f68fa0a2b 100644
--- a/drivers/input/misc/isa1200.c
+++ b/drivers/input/misc/isa1200.c
@@ -253,15 +253,12 @@ static void isa1200_stop(struct isa1200 *isa)
static void isa1200_play_work(struct work_struct *work)
{
struct isa1200 *isa = container_of(work, struct isa1200, play_work);
- struct input_dev *input = isa->input;
-
- scoped_guard(mutex_try, &input->mutex) {
- if (!isa->suspended) {
- if (isa->level)
- isa1200_start(isa);
- else
- isa1200_stop(isa);
- }
+
+ if (!READ_ONCE(isa->suspended)) {
+ if (isa->level)
+ isa1200_start(isa);
+ else
+ isa1200_stop(isa);
}
}
If this works no need to resend, I'll fold on my side.
Thanks.
--
Dmitry
^ permalink raw reply related
* Re: [PATCH v2 2/2] arm64: dts: qcom: sdm845-oneplus: Update compatible to include model
From: David Heidelberg @ 2026-06-25 18:39 UTC (permalink / raw)
To: Dmitry Torokhov, Krzysztof Kozlowski
Cc: Krzysztof Kozlowski, Konrad Dybcio, Rob Herring, Conor Dooley,
Jason A. Donenfeld, Matthias Schiffer, Vincent Huang,
Bjorn Andersson, Konrad Dybcio, linux-input, devicetree,
linux-kernel, linux-arm-msm, phone-devel
In-Reply-To: <aj1OhZQjO5nNYlAo@google.com>
On 25/06/2026 18:57, Dmitry Torokhov wrote:
> Hi Krzysztof,
>
> On Thu, Jun 25, 2026 at 10:23:54AM +0200, Krzysztof Kozlowski wrote:
>> On 25/06/2026 06:53, Dmitry Torokhov wrote:
>>> On Wed, Jun 24, 2026 at 04:37:25PM +0200, David Heidelberg wrote:
>>>> On 24/06/2026 06:28, Dmitry Torokhov wrote:
>>>>> Hi David,
>>>>>
>>>>> On Sun, Jun 21, 2026 at 07:11:45PM +0200, David Heidelberg wrote:
>>>>>> On 28/05/2026 00:13, David Heidelberg wrote:
>>>>>>> On 27/05/2026 23:56, Dmitry Torokhov wrote:
>>>>>>>> Hi David,
>>>>>>>>
>>>>>>>> On Sat, May 23, 2026 at 11:45:35AM +0200, David Heidelberg via B4 Relay wrote:
>>>>>>>>> From: David Heidelberg <david@ixit.cz>
>>>>>>>>>
>>>>>>>>> We know the driver is reporting s3706b, introduce the compatible so we
>>>>>>>>> can more easily introduce quirks for weird touchscreen replacements in
>>>>>>>>> followup series.
>>>>>>>>>
>>>>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
>>>>>>>>> Signed-off-by: David Heidelberg <david@ixit.cz>
>>>>>>>>> ---
>>>>>>>>> arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi | 2 +-
>>>>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>>>>>>>> b/arch/ arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>>>>>>>> index 6b7378cf4d493..148164d456a5a 100644
>>>>>>>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>>>>>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
>>>>>>>>> @@ -475,17 +475,17 @@ bq27441_fg: bq27441-battery@55 {
>>>>>>>>> };
>>>>>>>>> };
>>>>>>>>> &i2c12 {
>>>>>>>>> status = "okay";
>>>>>>>>> clock-frequency = <400000>;
>>>>>>>>> synaptics-rmi4-i2c@20 {
>>>>>>>>> - compatible = "syna,rmi4-i2c";
>>>>>>>>> + compatible = "syna,rmi4-s3706b", "syna,rmi4-i2c";
>>>>>>>>
>>>>>>>> So I believe we established that this device (s3706b) does not in fact
>>>>>>>> implement rmi4 protocol properly. Why do we have "syna,rmi4-i2c" as a
>>>>>>>> fallback? Shouldn't it be just "syna,rmi4-s3706b"?
>>>>>>>
>>>>>>> The vendor supplies s3706b which does implement the RMI4 properly.
>>>>>>>
>>>>>>> The 3rd party replacement impersonating original parts may not implement
>>>>>>> it properly, but I don't address this issue in this initial submission.
>>>>>>>
>>>>>>> With this compatible we know which original part is used by the vendor
>>>>>>> and installed in the phones, so later we can deduct specific sequences
>>>>>>> for the replacement aftermarket parts to keep phone touchscreen working
>>>>>>> same as they do on Android without affecting other devices.
>>>>>>
>>>>>> Hello Dmitry.
>>>>>>
>>>>>> May I ask what is currently preventing this series from moving forward?
>>>>>>
>>>>>> The first version was posted in 2023 [1]. I picked it up again in 2025 [2]
>>>>>> and am now on the 9th iteration (this patchset). At this point, the series
>>>>>> has been under discussion for well over a year, with relatively little
>>>>>> feedback and increasingly long gaps between review rounds.
>>>>>>
>>>>>> The current approach is based on the guidance I have received so far,
>>>>>> including suggestions from the device-tree maintainers. When concerns were
>>>>>> raised, I tried to address them and rework the series accordingly.
>>>>>>
>>>>>> What I am struggling with is understanding what specific issue still needs
>>>>>> to be resolved before these patches can be accepted. If there are remaining
>>>>>> requirements, objections to the approach, or technical concerns that I have
>>>>>> not addressed, I would appreciate having them stated explicitly so I can
>>>>>> work on them.
>>>>>>
>>>>>> I also split out the straightforward, self-contained changes in the hope
>>>>>> that at least those could progress independently while I continued working
>>>>>> on any follow-up requirements. However, even those patches do not appear to
>>>>>> be moving forward.
>>>>>>
>>>>>> Could you please clarify what outcome you would like to see from this
>>>>>> series, and what concrete changes would be required to get it accepted?
>>>>>
>>>>> I am still confused about how you want to differentiate between the full
>>>>> RMI4 support vs the OnePlus flavor. The "syna,rmi4-s3706b", as you
>>>>> mentioned, implements RMI4 protocol properly, so we do not need to
>>>>> actually have it documented neither in binding nor in DTS.
>>>>
>>>> --- part 1 ---
>>>>
>>>> This series addresses identification within device-tree. It's normal
>>>> recommended practice.
>>>>
>>>> If we know, the device ships specific, but **compliant** variant, we just
>>>> put it as compatible = "more-specific", "less-specific"; in this case
>>>> "syna,rmi4-s3706b", "syna,rmi4-i2c"
>>>>
>>>> This approach is used everywhere. This has nothing to do with after-market parts.
>>>
>>> We do this in many cases, sometimes when a part has different timings or
>>> maybe additional functionality compared to the base model.
>>
>> Generic expectation is to have always dedicated front compatible for
>> every device. rmi4-i2c is not really specific enough, more like a
>> family, thus a specific device compatible is essential by the DT rules.
>
> Essential in what way? What will break if such compatible is not there?
> We have lived without it for many years and will continue live happily
> without it for years to come.
Hi Dmitry, Krzystof,
Device tree should describe the hardware, rmi4-i2c isn't the exact model of
hardware used, the real hardware is Synaptics S3706B. Device-tree should, where
possible, describe the actual hardware used.
>
> We keep having this conversation each time there is self-describing
> protocol that does not require knowledge of a specific part number:
> i2c-hid, rmi4, spi-hid coming over soon.
While the protocol doesn't require this knowledge, where is the issue provide
the model, at least in the places where we know it?
Does it making things worse to describe hardware in more detail?
>
> We might need a device-specific compatible if we need to implement
> particular power on sequence/adjust timings, and that's when it starts
> making sense to introduce one.
>
>>
>> It does not matter if that specific compatible is ever used.
>>
>>>
>>> How does this new compatible for controller that fully implements RMI4
>>> protocol help here?
>>
>> It does not matter. This is a different device, thus it needs
>> front-specific compatible.
>
> Different from what?
>
> $ git grep syna,rmi4 -- arch/ | wc -l
> 43
>
> Do you have plans to list each and every chip currently covered by
> syna,rmi4* ?
Not really. I would definitely do the chips we know the model and encourage
others to identify chips their devices use, so developers know which hardware is
present. Each vendor should know which touchscreen model they ship with and
provide this information.
Reading the model from the dmesg at runtime is the least optimal way in my eyes.
>
>>
>> Also, the commit msg actually did mention how this helps: allowing
>> further quirks (I did not verify that in practice, but explanation is
>> plausible).
>
> Well, the devil is in the details. And that is what I am trying to
> understand.
This detail is irrelevant for this patchset. This patchset makes what
device-tree should do - describe the hardware independently on any future
patches which may use it in the future.
>
>>
>>>
>>>>
>>>> --- part 2 (irrelevant for this series) ---
>>>>
>>>>>
>>>>> The issue you have with after-market parts that are not compliant and we
>>>>> need to figure out how to deal with them. Inside the driver I
>>>>
>>>> As was suggested by device-tree folks, this is the first step, there isn't
>>>> better one available. If there is, please suggest one, and I'll apply it.
>>>
>>> Was it clearly communicated to DT folks that the compatible you are
>>> adding is fully compatible with the base "syna,rmi4-i2c" but other ones
>>> will not be compatible?
>>
>> That was not communicated but also did not have to. You can install in
>> your board whatever you wish, e.g. replacing foo device with bar being
>> something completely different and incompatible. Does not matter really
>> if this is after-market or a person just swapped things.
>>
>> DT does not solve that problem simply, because we describe static
>> hardware configuration.
>
> But the core issue that David is trying to solve is the fact that these
> headsets do not work well with aftermarket parts with the upstream
> kernels. It is not a theoretical problem for him, it is something that
> he's been trying to solve for a while.
As you can see, I'm trying solve different problem here, and that's describing
the hardware model.
>
> However from my POV I need to make sure the changes to the driver do not
> affect or limit well-behaved devices implementing RMI4 protocol
> properly.
I think we can agree that defining model of touchscreen used in the device-tree
won't affect any hardware you care about.
I'll be more than happy discuss the after-market parts in relevant patchset
submission. These two patches are not introducing anything directly related to
after-market parts, more like opposite defining the hardware which is shipped
with the phones from manufacturer.
David
[...]
^ permalink raw reply
* Re: [PATCH v2 2/2] arm64: dts: qcom: sdm845-oneplus: Update compatible to include model
From: Dmitry Torokhov @ 2026-06-25 16:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: David Heidelberg, Krzysztof Kozlowski, Konrad Dybcio, Rob Herring,
Conor Dooley, Jason A. Donenfeld, Matthias Schiffer,
Vincent Huang, Bjorn Andersson, Konrad Dybcio, linux-input,
devicetree, linux-kernel, linux-arm-msm, phone-devel
In-Reply-To: <f81e4d83-90d9-47c8-aee9-319df3f8b0fb@kernel.org>
Hi Krzysztof,
On Thu, Jun 25, 2026 at 10:23:54AM +0200, Krzysztof Kozlowski wrote:
> On 25/06/2026 06:53, Dmitry Torokhov wrote:
> > On Wed, Jun 24, 2026 at 04:37:25PM +0200, David Heidelberg wrote:
> >> On 24/06/2026 06:28, Dmitry Torokhov wrote:
> >>> Hi David,
> >>>
> >>> On Sun, Jun 21, 2026 at 07:11:45PM +0200, David Heidelberg wrote:
> >>>> On 28/05/2026 00:13, David Heidelberg wrote:
> >>>>> On 27/05/2026 23:56, Dmitry Torokhov wrote:
> >>>>>> Hi David,
> >>>>>>
> >>>>>> On Sat, May 23, 2026 at 11:45:35AM +0200, David Heidelberg via B4 Relay wrote:
> >>>>>>> From: David Heidelberg <david@ixit.cz>
> >>>>>>>
> >>>>>>> We know the driver is reporting s3706b, introduce the compatible so we
> >>>>>>> can more easily introduce quirks for weird touchscreen replacements in
> >>>>>>> followup series.
> >>>>>>>
> >>>>>>> Reviewed-by: Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>
> >>>>>>> Signed-off-by: David Heidelberg <david@ixit.cz>
> >>>>>>> ---
> >>>>>>> arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi | 2 +-
> >>>>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> b/arch/ arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> index 6b7378cf4d493..148164d456a5a 100644
> >>>>>>> --- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> +++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
> >>>>>>> @@ -475,17 +475,17 @@ bq27441_fg: bq27441-battery@55 {
> >>>>>>> };
> >>>>>>> };
> >>>>>>> &i2c12 {
> >>>>>>> status = "okay";
> >>>>>>> clock-frequency = <400000>;
> >>>>>>> synaptics-rmi4-i2c@20 {
> >>>>>>> - compatible = "syna,rmi4-i2c";
> >>>>>>> + compatible = "syna,rmi4-s3706b", "syna,rmi4-i2c";
> >>>>>>
> >>>>>> So I believe we established that this device (s3706b) does not in fact
> >>>>>> implement rmi4 protocol properly. Why do we have "syna,rmi4-i2c" as a
> >>>>>> fallback? Shouldn't it be just "syna,rmi4-s3706b"?
> >>>>>
> >>>>> The vendor supplies s3706b which does implement the RMI4 properly.
> >>>>>
> >>>>> The 3rd party replacement impersonating original parts may not implement
> >>>>> it properly, but I don't address this issue in this initial submission.
> >>>>>
> >>>>> With this compatible we know which original part is used by the vendor
> >>>>> and installed in the phones, so later we can deduct specific sequences
> >>>>> for the replacement aftermarket parts to keep phone touchscreen working
> >>>>> same as they do on Android without affecting other devices.
> >>>>
> >>>> Hello Dmitry.
> >>>>
> >>>> May I ask what is currently preventing this series from moving forward?
> >>>>
> >>>> The first version was posted in 2023 [1]. I picked it up again in 2025 [2]
> >>>> and am now on the 9th iteration (this patchset). At this point, the series
> >>>> has been under discussion for well over a year, with relatively little
> >>>> feedback and increasingly long gaps between review rounds.
> >>>>
> >>>> The current approach is based on the guidance I have received so far,
> >>>> including suggestions from the device-tree maintainers. When concerns were
> >>>> raised, I tried to address them and rework the series accordingly.
> >>>>
> >>>> What I am struggling with is understanding what specific issue still needs
> >>>> to be resolved before these patches can be accepted. If there are remaining
> >>>> requirements, objections to the approach, or technical concerns that I have
> >>>> not addressed, I would appreciate having them stated explicitly so I can
> >>>> work on them.
> >>>>
> >>>> I also split out the straightforward, self-contained changes in the hope
> >>>> that at least those could progress independently while I continued working
> >>>> on any follow-up requirements. However, even those patches do not appear to
> >>>> be moving forward.
> >>>>
> >>>> Could you please clarify what outcome you would like to see from this
> >>>> series, and what concrete changes would be required to get it accepted?
> >>>
> >>> I am still confused about how you want to differentiate between the full
> >>> RMI4 support vs the OnePlus flavor. The "syna,rmi4-s3706b", as you
> >>> mentioned, implements RMI4 protocol properly, so we do not need to
> >>> actually have it documented neither in binding nor in DTS.
> >>
> >> --- part 1 ---
> >>
> >> This series addresses identification within device-tree. It's normal
> >> recommended practice.
> >>
> >> If we know, the device ships specific, but **compliant** variant, we just
> >> put it as compatible = "more-specific", "less-specific"; in this case
> >> "syna,rmi4-s3706b", "syna,rmi4-i2c"
> >>
> >> This approach is used everywhere. This has nothing to do with after-market parts.
> >
> > We do this in many cases, sometimes when a part has different timings or
> > maybe additional functionality compared to the base model.
>
> Generic expectation is to have always dedicated front compatible for
> every device. rmi4-i2c is not really specific enough, more like a
> family, thus a specific device compatible is essential by the DT rules.
Essential in what way? What will break if such compatible is not there?
We have lived without it for many years and will continue live happily
without it for years to come.
We keep having this conversation each time there is self-describing
protocol that does not require knowledge of a specific part number:
i2c-hid, rmi4, spi-hid coming over soon.
We might need a device-specific compatible if we need to implement
particular power on sequence/adjust timings, and that's when it starts
making sense to introduce one.
>
> It does not matter if that specific compatible is ever used.
>
> >
> > How does this new compatible for controller that fully implements RMI4
> > protocol help here?
>
> It does not matter. This is a different device, thus it needs
> front-specific compatible.
Different from what?
$ git grep syna,rmi4 -- arch/ | wc -l
43
Do you have plans to list each and every chip currently covered by
syna,rmi4* ?
>
> Also, the commit msg actually did mention how this helps: allowing
> further quirks (I did not verify that in practice, but explanation is
> plausible).
Well, the devil is in the details. And that is what I am trying to
understand.
>
> >
> >>
> >> --- part 2 (irrelevant for this series) ---
> >>
> >>>
> >>> The issue you have with after-market parts that are not compliant and we
> >>> need to figure out how to deal with them. Inside the driver I
> >>
> >> As was suggested by device-tree folks, this is the first step, there isn't
> >> better one available. If there is, please suggest one, and I'll apply it.
> >
> > Was it clearly communicated to DT folks that the compatible you are
> > adding is fully compatible with the base "syna,rmi4-i2c" but other ones
> > will not be compatible?
>
> That was not communicated but also did not have to. You can install in
> your board whatever you wish, e.g. replacing foo device with bar being
> something completely different and incompatible. Does not matter really
> if this is after-market or a person just swapped things.
>
> DT does not solve that problem simply, because we describe static
> hardware configuration.
But the core issue that David is trying to solve is the fact that these
headsets do not work well with aftermarket parts with the upstream
kernels. It is not a theoretical problem for him, it is something that
he's been trying to solve for a while.
However from my POV I need to make sure the changes to the driver do not
affect or limit well-behaved devices implementing RMI4 protocol
properly.
>
> >
> >>
> >>> essentially need a"incomplete protocol" flag that we can use to
> >>> implement additional checks or skip known to be not implemented
> >>> functions/queries. In DT we could introduce something like
> >>> "oneplus,rmi4-i2c" that is decidedly not compatible with "syna,rmi4-i2c"
> >>> and neither one should be a fallback for the other.
> >>>
> >>> This of course needs buy-in from DT maintainers.
> >>
> >> As you can see, this still holds Acked-by and Reviewed-by from the relevant
> >> people - Krzysztof and Konrad.
> >
> > I see that but the commit does not explain how exactly you are planning
> > to deal with knockoffs.
>
> I think it does not have to. David does not need to solve all possible
> problems. He is solving his problems and not preventing the other
> problems to be solved in the future. Unless you expect that the second
> part - future problems - will be blocked by this code.
Well, as I mentioned above my understanding is that David really wants
to solve the issue with after market parts not working properly.
>
> But then I think no one ever solved in DT a problem of replacement of
> valid parts with after-market incompatible parts. Therefore I do not
> even know what solution I could suggest to David.
>
> >
> >>
> >>>
> >>> Does this make sense?
> >>
> >> For the scope we're discussing it doesn't seems so.
> >>
> >> This discussion should be associated with the last revision of the full
> >> series I sent 3 months ago. We're in very unflattering state, where:
> >>
> >> 2018 - these aftermarket touchscreen worked on Android well enough for
> >> people to have working touch (let's say with slightly worse experience then
> >> the original).
> >>
> >> 2026 in the mainline, we cannot even more forward and report to user-space
> >> there is aftermarket non-compliant piece of hardware installed.
> >>
> >> Actionable steps I suggest after this series lands:
> >>
> >> 1. don't do any changes, but since we know what 3rd party touchscreen do
> >> incorrectly deviating from the standard, REPORT it to the userspace, so USER
> >> know, their device (phone/tablet) doesn't have original part.
> >>
> >> 2. then figure out, IF we can reasonably well workaround it and HOW to do it
> >>
> >> These two steps present some progress which could be discussed and could
> >> lead us somewhere, what do you think?
> >
> > So since we know that these devices can come with controllers that do
> > not implement RMI4 fully, can we:
> >
> > 1. Establish a new compatible that is separate from syna,rmi4-i2c? As I
> > mentioned, it could be oneplus,rmi4-i2c or event a concrete controller
> > vendor,id combo. The point that it should be completely separate from
> > the current compatible and not use the current compatible as a fallback.
>
> Board/machine level compatible already gives you that.
> after-market-incompatible-parts is not a problem of this s3706b, if I
> understood correctly. It feels to me more of a problem of the board:
> people install into this board some incompatible replacement parts.
So is the suggestion to check if the board matches "oneplus,enchilada",
"oneplus,fajitas" with of_machine_compatible_match()?
>
> I would understand that this is not a board/machine level problem, if
> multiple different boards with s3706b were affected. Is this the case?
David?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: usbhid: Use seq_buf_putc() in three functions
From: Markus Elfring @ 2026-06-25 16:55 UTC (permalink / raw)
To: sashiko-reviews, linux-input, linux-usb, Benjamin Tissoires,
Jiri Kosina, Mahad Ibrahim
Cc: Dmitry Torokhov, Steven Rostedt, Woradorn Laodhanadhaworn, LKML,
kernel-janitors
In-Reply-To: <20260625093209.34B6B1F000E9@smtp.kernel.org>
>> +++ b/drivers/hid/usbhid/hid-core.c
>> @@ -1412,7 +1412,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>>
>> if (dev->product) {
>> if (dev->manufacturer)
>> - seq_buf_puts(&hid_name, " ");
>> + seq_buf_putc(&hid_name, ' ');
>
> [Severity: Medium]
> Does this change introduce a missing null termination bug in usbhid_probe()
> if the manufacturer string is exactly 127 bytes long?
>
> The previous code using seq_buf_puts() requires two bytes of available
> capacity (one for the space, one for the trailing null byte) and safely fails
> if there isn't enough room.
I see a need to reconsider such information in more detail.
> Looking at seq_buf_putc(), it only checks if there is room for one byte.
> If the sequence buffer is exactly 128 bytes long
This is the case as such a technical detail can be determined from the member “name”
of the data structure “hid_device”.
https://elixir.bootlin.com/linux/v7.1.1/source/include/linux/hid.h#L721
> and already contains 127
> characters from dev->manufacturer plus the null terminator, seq_buf_putc()
> will overwrite the null terminator at index 127 with a space character.
Other contributors can add further experiences for corresponding data length characteristics.
> This leaves hid->name completely unterminated.
I would like to point the comment “Don't count the trailing null byte against the capacity”
once more from the implementation of the function “seq_buf_puts”.
https://elixir.bootlin.com/linux/v7.1.1/source/lib/seq_buf.c#L193-L194
> When exposed to userspace via
> sysfs or input ioctls, this could lead to an out-of-bounds read into the
> adjacent hid->phys memory field.
If such a data reuse would be attempted, API requirements for sequence buffers
should probably be taken better into account at a concrete place.
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v2] iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()
From: sashiko-bot @ 2026-06-25 12:39 UTC (permalink / raw)
To: Sanjay Chitroda; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260625121611.2003780-1-sanjayembedded@gmail.com>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The driver incorrectly uses a static global structure `temperature_callbacks` to hold per-device state, causing cross-device data corruption and potential Use-After-Free.
- [High] A race condition during device unbind can lead to a Use-After-Free or NULL pointer dereference because `sensor_hub_remove_callback` does not wait for concurrently executing events.
--
commit 0a06e7ba97cafc3e2c79740421bf8186cb0fc540
Author: Sanjay Chitroda <sanjayembeddedse@gmail.com>
iio: temperature: hid-sensor-temperature: switch to non-devm iio_device_register()
The patch switches from devm_iio_device_register() to iio_device_register()
to fix a teardown ordering issue. It adds an explicit iio_device_unregister()
in the remove function to ensure the device is not accessed after its backend
resources begin to be dismantled.
> diff --git a/drivers/iio/temperature/hid-sensor-temperature.c b/drivers/iio/temperature/hid-sensor-temperature.c
> index 9f628a8e5cfbe..34bff7e9f3a33 100644
> --- a/drivers/iio/temperature/hid-sensor-temperature.c
> +++ b/drivers/iio/temperature/hid-sensor-temperature.c
> @@ -244,7 +244,7 @@ static int hid_temperature_probe(struct platform_device *pdev)
> if (ret)
> goto error_remove_trigger;
>
> - ret = devm_iio_device_register(indio_dev->dev.parent, indio_dev);
> + ret = iio_device_register(indio_dev);
[Severity: High]
This is a pre-existing issue, but does this driver incorrectly use a static
global structure for per-device state?
Looking at hid_temperature_probe(), we overwrite the shared global pdev
pointer in the statically allocated temperature_callbacks structure:
drivers/iio/temperature/hid-sensor-temperature.c:hid_temperature_probe() {
...
temperature_callbacks.pdev = pdev;
ret = sensor_hub_register_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE,
&temperature_callbacks);
...
}
If multiple temperature sensors are probed, the most recently probed device
will overwrite this shared pointer. If the last probed device is later
unbound, its driver data is cleared.
Subsequent events for other active devices will dereference the cleared pdev
in temperature_proc_event(), leading to a null pointer dereference or
use-after-free.
> if (ret)
> goto error_remove_callback;
>
> @@ -264,6 +264,7 @@ static void hid_temperature_remove(struct platform_device *pdev)
> struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> struct temperature_state *temp_st = iio_priv(indio_dev);
>
> + iio_device_unregister(indio_dev);
> sensor_hub_remove_callback(hsdev, HID_USAGE_SENSOR_TEMPERATURE);
[Severity: High]
This isn't a bug introduced by this patch, but can a race condition during
device unbind lead to a use-after-free or null pointer dereference?
The sensor_hub_remove_callback() removes the callback from the sensor hub's
list, but does not synchronize with concurrently executing callback handlers
like sensor_hub_raw_event().
If an event from the sensor hub arrives exactly as the driver is unbinding,
the event handler might fetch the callback just before it is removed. When
hid_temperature_remove() completes, devres frees indio_dev, and the driver
core sets drvdata to NULL.
The concurrent event handler then executes temperature_capture_sample(),
calling platform_get_drvdata(pdev) which either returns NULL or a dangling
pointer to the freed indio_dev, causing a crash.
> hid_sensor_remove_trigger(indio_dev, &temp_st->common_attributes);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260625121611.2003780-1-sanjayembedded@gmail.com?part=1
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox