* [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: Tianchu Chen @ 2026-05-29 13:42 UTC (permalink / raw)
To: jikos, bentiss; +Cc: linux-input, linux-kernel, stable
From: Tianchu Chen <flynnnchen@tencent.com>
goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
buffer (tmp_buf), writing an 11-12 byte header followed by the
caller-supplied report data. The HID core caps report size at
HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
hid_ll_driver.max_buffer_size and performs no bounds checking before
copying the payload:
memcpy(tmp_buf + tx_len, buf, len);
A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
overflows the stack buffer.
Add a size check after constructing the header, rejecting reports that
would exceed the buffer capacity.
Discovered by Atuin - Automated Vulnerability Discovery Engine.
Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
drivers/hid/hid-goodix-spi.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
index 80c0288a3..288cb827e 100644
--- a/drivers/hid/hid-goodix-spi.c
+++ b/drivers/hid/hid-goodix-spi.c
@@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
memcpy(tmp_buf + tx_len, args, args_len);
tx_len += args_len;
+ if (tx_len + len > sizeof(tmp_buf))
+ return -EINVAL;
+
memcpy(tmp_buf + tx_len, buf, len);
tx_len += len;
--
2.51.0
^ permalink raw reply related
* Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: sashiko-bot @ 2026-05-29 14:27 UTC (permalink / raw)
To: Tianchu Chen; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Medium] The patch artificially caps SET_REPORT payload size to a 128-byte stack buffer, potentially breaking legitimate large HID reports.
Pre-existing issues:
- [High] Concurrent reallocation of `ts->event_buf` in `goodix_hid_start` and access in `goodix_hid_irq` leads to a Use-After-Free.
- [Medium] Uninitialized kernel memory is transmitted over the SPI bus during read operations.
- [Medium] The driver performs SPI reads and writes without verifying the hardware suspend state.
- [Medium] In `goodix_spi_resume()`, the IRQ is enabled before the device is powered on.
- [Medium] Unsafe assignment of `devm_krealloc` directly to the buffer pointer causes a device-lifetime memory leak.
--
commit a3440b67c988b5226da753835060ad3d93591c74
Author: Tianchu Chen <flynnnchen@tencent.com>
Subject: HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
This commit adds a bounds check in goodix_hid_set_raw_report() to ensure that
the constructed protocol frame does not exceed the size of the 128-byte stack
buffer tmp_buf. If a caller supplies a report that exceeds this limit, the
function now returns -EINVAL to prevent a stack buffer overflow.
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3a38b..288cb827e9d6d 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
> memcpy(tmp_buf + tx_len, args, args_len);
> tx_len += args_len;
>
> + if (tx_len + len > sizeof(tmp_buf))
> + return -EINVAL;
[Severity: Medium]
Does returning `-EINVAL` here artificially cap the SET_REPORT payload size?
If the device's HID report descriptor defines larger output or feature reports
(e.g., for firmware updates or configuration), would this fix block them?
Could we dynamically allocate a buffer of adequate size or use the already
available 2KB `ts->xfer_buf` instead of relying on a hardcoded stack limit?
> +
> memcpy(tmp_buf + tx_len, buf, len);
> tx_len += len;
>
[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk with
`ts->event_buf` between `goodix_hid_irq()` and `goodix_hid_start()`?
If the HID driver is unbound and rebound, `goodix_hid_start()` will execute
`devm_krealloc()`, freeing the old `ts->event_buf`:
drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
...
ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
report_size, GFP_KERNEL);
...
}
Meanwhile, if `goodix_hid_close()` clears the `GOODIX_HID_STARTED` flag but
does not synchronize with the threaded IRQ, a preempted IRQ handler can pass
the flag check and then use the now-freed `ts->event_buf`:
drivers/hid/hid-goodix-spi.c:goodix_hid_irq() {
...
if (!test_bit(GOODIX_HID_STARTED, &ts->flags))
return IRQ_HANDLED;
...
event = goodix_get_event_report(ts, ts->hid_report_addr, ts->event_buf,
GOODIX_HID_ACK_HEADER_SIZE +
GOODIX_HID_COOR_PKG_LEN);
...
}
[Severity: Medium]
This is a pre-existing issue, but does assigning the result of
`devm_krealloc()` directly to `ts->event_buf` cause a memory leak if it fails?
drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
...
ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
report_size, GFP_KERNEL);
...
}
If `devm_krealloc()` returns `NULL`, the original memory chunk remains in the
`devres` list, but the pointer in `ts->event_buf` is overwritten. Subsequent
re-binds would allocate a new devres chunk instead of reusing the old one,
accumulating leaked chunks until the SPI device is unbound.
[Severity: Medium]
This is a pre-existing issue, but does this transmit uninitialized kernel
memory over the SPI MOSI line during read operations?
In `goodix_get_event_report()`, `ts->event_buf` is used as the transaction
buffer, and it is allocated with `devm_kmalloc()` (which doesn't zero memory).
The first 5 bytes are initialized, but the dummy bytes and read payload
section are left uninitialized:
drivers/hid/hid-goodix-spi.c:goodix_get_event_report() {
...
data[0] = GOODIX_SPI_READ_FLAG;
put_unaligned_be32(addr, data + GOODIX_SPI_TRANS_PREFIX_LEN);
spi_message_init(&spi_msg);
memset(&xfers, 0, sizeof(xfers));
xfers.tx_buf = data;
xfers.rx_buf = data;
xfers.len = GOODIX_SPI_READ_PREFIX_LEN + len;
...
}
Because `xfers.tx_buf` is provided for the entire transfer length, is the
uninitialized memory sent to the hardware during the dummy and read phases,
resulting in an information leak?
[Severity: Medium]
This is a pre-existing issue, but does `goodix_hid_raw_request()` perform SPI
operations without verifying the hardware suspend state?
drivers/hid/hid-goodix-spi.c:goodix_hid_raw_request() {
...
guard(mutex)(&ts->hid_request_lock);
switch (reqtype) {
case HID_REQ_GET_REPORT:
error = goodix_hid_get_raw_report(hid, reportnum, buf,
len, rtype);
break;
case HID_REQ_SET_REPORT:
if (buf[0] == reportnum)
error = goodix_hid_set_raw_report(hid, reportnum,
buf, len, rtype);
break;
...
}
If a request is issued while the device is in the `GOODIX_SPI_POWER_SLEEP`
state, the driver will directly perform SPI read/write operations on the
powered-down hardware. Could this cause timeouts or undefined hardware states?
[Severity: Medium]
This is a pre-existing issue, but should the IRQ be enabled before the device
is powered on during resume?
drivers/hid/hid-goodix-spi.c:goodix_spi_resume() {
...
enable_irq(ts->spi->irq);
return goodix_spi_set_power(ts, GOODIX_SPI_POWER_ON);
}
If a spurious interrupt or power-on glitch asserts the IRQ line while the
device is still in the sleep state, the interrupt handler will execute and
attempt to perform SPI reads on the waking hardware. Would it be safer to
bring the device to an active power state before unmasking its interrupts?
--
Sashiko AI review · https://sashiko.dev/#/patchset/f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev?part=1
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing
From: Hans de Goede @ 2026-05-29 15:00 UTC (permalink / raw)
To: 谢致邦 (XIE Zhibang), linux-input
Cc: bentiss, dianders, jikos, linux-kernel, superm1
In-Reply-To: <tencent_ECC71F91403F5AC69E1E6B963FA667ABF607@qq.com>
Hi,
On 29-May-26 14:16, 谢致邦 (XIE Zhibang) wrote:
> Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
> separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
> devices and hid-over-i2c OF devices. After the split, devices with _HID
> "PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
> i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
> for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
> descriptor address only through the _DSM method. Fall back to the _DSM
> call when the property is absent.
>
> Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
Thank you for the new patch, this is an interesting approach and better
then the modalias magic from the previous version.
Note I'm not the i2c-hid maintainer, with that said I think this should
be acceptable. But currently it duplicates the _DSM handling code and
that should be fixed.
I think this should be changed to a series of 3 patches:
1. Move the i2c_hid_acpi_blacklist handling out of
i2c_hid_acpi_get_descriptor() into i2c_hid_acpi_probe()
to above the devm_kzalloc() call.
2. Move i2c_hid_acpi_get_descriptor() to a generic
int i2c_hid_acpi_get_descriptor(struct device *dev)
helper in drivers/hid/i2c-hid/i2c-hid-core.c .
Wrapped in #ifdef CONFIG_ACPI and with a static inline
stub in drivers/hid/i2c-hid/i2c-hid.h when CONFIG_ACPI
is not set, e.g. in drivers/hid/i2c-hid/i2c-hid.h add:
#ifdef CONFIG_ACPI
int i2c_hid_acpi_get_descriptor_address(struct device *dev);
#else
static inline int i2c_hid_acpi_get_descriptor_address(struct device *dev)
{
return -ENODEV;
}
#endif
3. Modify i2c-hid-of.c to try i2c_hid_acpi_get_descriptor_address() as
fallback for the missing "hid-descr-addr" property. Please also add
a comment in the code explaining that this fallback is about ACPI I2C-hid
devices which use a "PRP0001" ACPI _HID with an "hid-over-i2c" compatible.
Regards,
Hans
> ---
> drivers/hid/i2c-hid/i2c-hid-of.c | 44 ++++++++++++++++++++++++++++++++
> 1 file changed, 44 insertions(+)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> index 57379b77e977..62c089a6455a 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-of.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> @@ -19,6 +19,7 @@
> * more details.
> */
>
> +#include <linux/acpi.h>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/gpio/consumer.h>
> @@ -74,6 +75,39 @@ static void i2c_hid_of_power_down(struct i2chid_ops *ops)
> ihid_of->supplies);
> }
>
> +#ifdef CONFIG_ACPI
> +/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
> +static guid_t i2c_hid_of_acpi_guid =
> + GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
> + 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
> +
> +/*
> + * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
> + * declare their I2C HID touchpad with _HID "PRP0001" and _DSD compatible
> + * "hid-over-i2c" but lack the "hid-descr-addr" property. Use the _DSM
> + * method to obtain the HID descriptor address.
> + */
> +static int i2c_hid_of_acpi_get_descriptor(struct device *dev)
> +{
> + struct acpi_device *adev = ACPI_COMPANION(dev);
> + union acpi_object *obj;
> + u16 addr;
> +
> + if (!adev)
> + return -ENODEV;
> +
> + obj = acpi_evaluate_dsm_typed(acpi_device_handle(adev),
> + &i2c_hid_of_acpi_guid, 1, 1, NULL,
> + ACPI_TYPE_INTEGER);
> + if (!obj)
> + return -ENODEV;
> +
> + addr = obj->integer.value;
> + ACPI_FREE(obj);
> + return addr;
> +}
> +#endif
> +
> static int i2c_hid_of_probe(struct i2c_client *client)
> {
> struct device *dev = &client->dev;
> @@ -92,6 +126,16 @@ static int i2c_hid_of_probe(struct i2c_client *client)
> ihid_of->ops.power_down = i2c_hid_of_power_down;
>
> ret = device_property_read_u32(dev, "hid-descr-addr", &val);
> +#ifdef CONFIG_ACPI
> + if (ret) {
> + int dsm_ret = i2c_hid_of_acpi_get_descriptor(dev);
> +
> + if (dsm_ret >= 0) {
> + val = dsm_ret;
> + ret = 0;
> + }
> + }
> +#endif
> if (ret) {
> dev_err(dev, "HID register address not provided\n");
> return -ENODEV;
^ permalink raw reply
* Re: [PATCH v10 1/4] HID: hid-msi: Add MSI Claw configuration driver
From: Dmitry Torokhov @ 2026-05-29 18:29 UTC (permalink / raw)
To: Derek John Clark
Cc: Jiri Kosina, Benjamin Tissoires, Pierre-Loup A . Griffais,
Denis Benato, Zhouwang Huang, linux-input, linux-doc,
linux-kernel
In-Reply-To: <CAFqHKT=zRMW4gu09xz2WAukjXB0i9d-z-SfkxU67yJkCA0DZvQ@mail.gmail.com>
On Thu, May 28, 2026 at 11:34:18PM -0700, Derek John Clark wrote:
> On Wed, May 27, 2026 at 10:32 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Derek,
> >
> > On Wed, May 27, 2026 at 10:21:19PM +0000, Derek J. Clark wrote:
> > > Adds configuration HID driver for the MSI Claw series of handheld PC's.
> > > In this initial patch add the initial driver outline and attributes for
> > > changing the gamepad mode, M-key behavior, and add a WO reset function.
> > >
> > > Sending the SWITCH_MODE and RESET commands causes a USB disconnect in
> > > the device. The completion will therefore never get hit and would trigger
> > > an -EIO. To avoid showing the user an error for every write to these
> > > attrs a bypass for the completion handling is introduced when timeout ==
> > > 0.
> > >
> > > The initial version of this patch was written by Denis Benato, which
> > > contained the initial reverse-engineering and implementation for the
> > > gamepad mode switching. This work was later expanded by Zhouwang Huang
> > > to include more gamepad modes. Finally, I refactored the drivers data
> > > in/out flow and overall format to conform to kernel driver best
> > > practices and style guides. Claude was used as an initial reviewer of
> > > this patch.
> >
> > I wonder why do you need to roll asynchronous probing and asynchronous
> > resume by hand? This I think complicates the driver greatly and forces
> > you to use a ton of works, spinlocks, and checks.
> >
> > Thanks.
> >
>
> Hi Dmitry,
>
> I suppose being asked this means my cover letter and commit
> descriptions need some additional context. The MCU in these Claw
> devices is quite temperamental. There are a few specific issues that
> cause the need for multiple work queues, a serialization mutex, and
> subsequently spinlocks to prevent stale data reads.
>
> 1.) The MCU will halt function if it receives any output reports
> before ~500MS after probe or resume. This can either manifest as the
> device never responding to a command, or it can cause the entire
> system to become unstable and reboot. This creates the need for
> cfg_setup to query the MCU and then add the gamepad attrs, led_mc
> device, and rgb attrs. As a side effect, because a system could
> technically be suspended during that 500ms delay, there exists the
> need to re-queue the work if it was never triggered, hence the resume
> queue.
> 2.) The MCU will not always respond in order if two or more output
> reports are sent within a few ms of each other. Since many of the
> commands use a generic "ACK", or share an "ACK" type but don't provide
> specific context about what sub-function called them, we could
> potentially have cross talk where data is saved in the wrong attribute
> or errors propagate because of a missed message. To get around this
> serialization issue we hold a mutex through a completion triggered in
> raw_event and, for most events, save a state machine on what command
> is expected and what sub-command was the initiator. (I.E. profile
> events handle the M1, M2, RGB, Left rumble, and right rumble). Since
> the state machine is accessed on both sides, we need spinlocks
> guarding the reads. This essentially serializes the data and makes it
> predictable. Using this pattern I haven't had any issues reading from
> or writing to the MCU.
> 3.) Some commands will never return their "ACK" while a completion is
> held, so we have a workaround to basically ignore them and hope the
> command worked. This is only needed for SYNC_TO_ROM, from which we
> don't need to set anything on its "ACK", and switching the gamepad
> mode, which causes USB disconnect/reconnect and the driver fully
> reloads, so we'll never be able to read it anyway.
> 4.) The RGB work queue is used to free the userspace write while the
> completion is held. I found that use without it could stall userspace
> quite significantly if it has multiple writes back to back. I
> experienced this using Steam's customization menu, which sends a
> single write for every increment of its color and brightness sliders.
> when traversing the full length of the slider it is possible to have
> effects changing for nearly a minute after stopping. With the queue,
> only the most recent write is eventually sent to the MCU. This issue
> also affects the Go 2 driver as well, though not to the same extent,
> but for which I'll be adding a similar de-bounce queue soon. Go S is
> also technically affected by this bug, but that returns quickly enough
> that it isn't really feasible to trigger the bug with much frequency.
> I'll still fix that one as well though.
>
> TBH I'm not "happy" with the complexity of the driver, but I don't see
> a reasonable alternative. If you have any specific suggestions that I
> could try that might simplify it, I'd be more than happy to give it a
> shot. That being said, I'm not very optimistic about it. Development
> on this device has been like wrestling a bear.
Thank you for this detailed explanation. I would like to concentrate on
the #1 first. What happens in the driver is you are essentially rolling
asynchronous probing and asynchronous suspend/resume in the driver
itself, and end up fighting with the kernel and the driver core
specifically.
As far as suspend/resume goes: HID subsystem already enables
asynchronous resume handling (checkout the call to
device_enable_async_suspend() in
drivers/hid/hid-core.c::hid_allocate_device()). Therefore I think you
just need to stick the necessary delay in your resume method() and call
it a day.
For the probing I would look into annotating the driver as
PROBE_PREFER_ASYNCHRONOUS and relying on that. Again, if you stick the
required delay in probe then sysfs attributes will not be created too
early, same for the rest of concerns with the device being exposed to
userspace before it is ready to handle requests.
If there are issues with HID subsystem honoring
PROBE_PREFER_ASYNCHRONOUS I would look into fixing the subsystem rather
than try to work around it in the driver.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow
From: Dmitry Torokhov @ 2026-05-29 19:31 UTC (permalink / raw)
To: Tianchu Chen; +Cc: jikos, bentiss, linux-input, linux-kernel, stable
In-Reply-To: <f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev>
On Fri, May 29, 2026 at 01:42:47PM +0000, Tianchu Chen wrote:
> From: Tianchu Chen <flynnnchen@tencent.com>
>
> goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
> buffer (tmp_buf), writing an 11-12 byte header followed by the
> caller-supplied report data. The HID core caps report size at
> HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
> hid_ll_driver.max_buffer_size and performs no bounds checking before
> copying the payload:
>
> memcpy(tmp_buf + tx_len, buf, len);
>
> A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
> overflows the stack buffer.
>
> Add a size check after constructing the header, rejecting reports that
> would exceed the buffer capacity.
>
> Discovered by Atuin - Automated Vulnerability Discovery Engine.
>
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
> ---
> drivers/hid/hid-goodix-spi.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3..288cb827e 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
> memcpy(tmp_buf + tx_len, args, args_len);
> tx_len += args_len;
>
> + if (tx_len + len > sizeof(tmp_buf))
> + return -EINVAL;
> +
We can also consider returning -E2BIG here.
> memcpy(tmp_buf + tx_len, buf, len);
> tx_len += len;
>
In any case:
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
I think we can ignore Sashiko's ramblings on this patch, it needs some
instructions detailing order of operations/timing of callbacks in HID
subsystem.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: i2c-hid-of: Fall back to ACPI _DSM when hid-descr-addr is missing
From: Dmitry Torokhov @ 2026-05-29 19:36 UTC (permalink / raw)
To: Hans de Goede
Cc: 谢致邦 (XIE Zhibang), linux-input, bentiss,
dianders, jikos, linux-kernel, superm1
In-Reply-To: <7c2c86d8-be10-4e8e-8e96-d22788ec488f@kernel.org>
On Fri, May 29, 2026 at 05:00:37PM +0200, Hans de Goede wrote:
> Hi,
>
> On 29-May-26 14:16, 谢致邦 (XIE Zhibang) wrote:
> > Before commit b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are
> > separate modules"), the unified i2c-hid driver handled both PNP0C50 ACPI
> > devices and hid-over-i2c OF devices. After the split, devices with _HID
> > "PRP0001" and _DSD compatible "hid-over-i2c" are only probed by
> > i2c_hid_of, which requires "hid-descr-addr" in the _DSD. Some devices,
> > for example the Lenovo KaiTian N60d and Inspur CP300L3, provide the HID
> > descriptor address only through the _DSM method. Fall back to the _DSM
> > call when the property is absent.
> >
> > Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
> > Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
>
> Thank you for the new patch, this is an interesting approach and better
> then the modalias magic from the previous version.
>
> Note I'm not the i2c-hid maintainer, with that said I think this should
> be acceptable. But currently it duplicates the _DSM handling code and
> that should be fixed.
>
> I think this should be changed to a series of 3 patches:
>
> 1. Move the i2c_hid_acpi_blacklist handling out of
> i2c_hid_acpi_get_descriptor() into i2c_hid_acpi_probe()
> to above the devm_kzalloc() call.
>
> 2. Move i2c_hid_acpi_get_descriptor() to a generic
> int i2c_hid_acpi_get_descriptor(struct device *dev)
> helper in drivers/hid/i2c-hid/i2c-hid-core.c .
> Wrapped in #ifdef CONFIG_ACPI and with a static inline
> stub in drivers/hid/i2c-hid/i2c-hid.h when CONFIG_ACPI
> is not set, e.g. in drivers/hid/i2c-hid/i2c-hid.h add:
>
> #ifdef CONFIG_ACPI
> int i2c_hid_acpi_get_descriptor_address(struct device *dev);
> #else
> static inline int i2c_hid_acpi_get_descriptor_address(struct device *dev)
> {
> return -ENODEV;
> }
> #endif
>
> 3. Modify i2c-hid-of.c to try i2c_hid_acpi_get_descriptor_address() as
> fallback for the missing "hid-descr-addr" property. Please also add
> a comment in the code explaining that this fallback is about ACPI I2C-hid
> devices which use a "PRP0001" ACPI _HID with an "hid-over-i2c" compatible.
I think we should also stick a big fat warning if _DSM succeeds in that
code branch: hopefully manufacturers will notice and fix the firmware on
new devices.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
From: Dmitry Torokhov @ 2026-05-29 21:33 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Jinmo Yang, linux-input
In-Reply-To: <20260528184231.1F0581F000E9@smtp.kernel.org>
On Thu, May 28, 2026 at 06:42:29PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
>
> Pre-existing issues:
> - [High] wacom_wac_queue_flush() allocates memory with GFP_KERNEL while executing in an atomic context, leading to a 'scheduling while atomic' panic.
> --
>
> commit 70e9615b93de485ba1816f95e27e25b054df4b6a
> Author: Jinmo Yang <jinmo44.yang@gmail.com>
>
> HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
>
> This commit fixes a slab-out-of-bounds write in wacom_wac_queue_insert().
> It prevents kfifo_skip() from operating on an empty fifo and validates
> the return value of kfifo_in().
>
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index a32320b351e3e..489ca68f1560f 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
>
> [ ... ]
>
> > @@ -62,7 +62,9 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> > kfifo_skip(fifo);
> > }
> >
> > - kfifo_in(fifo, raw_data, size);
> > + if (!kfifo_in(fifo, raw_data, size))
> > + hid_warn_ratelimited(hdev, "%s: report is too large (%d)\n",
> > + __func__, size);
> > }
> >
> > static void wacom_wac_queue_flush(struct hid_device *hdev,
> > struct kfifo_rec_ptr_2 *fifo)
> > {
> > while (!kfifo_is_empty(fifo)) {
> > int size = kfifo_peek_len(fifo);
> > u8 *buf;
> > unsigned int count;
> > int err;
> >
> > buf = kzalloc(size, GFP_KERNEL);
>
> [Severity: High]
> This is a pre-existing issue, but is it safe to use GFP_KERNEL here?
>
> Looking at the call chain, wacom_wac_queue_flush() is invoked via
> wacom_wac_pen_serial_enforce() from the driver's .raw_event callback,
> wacom_raw_event().
>
> For USB HID devices, this callback is typically triggered by hid_irq_in(),
> which is an URB completion handler that runs in an atomic context.
>
> Since using GFP_KERNEL allows the allocator to sleep, could this lead to a
> scheduling while atomic panic? Would it be better to use GFP_ATOMIC instead?
>
This looks like valid analysis. Would you be willing to send a
[completely separate] patch?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix slab-out-of-bounds write in wacom_wac_queue_insert
From: Dmitry Torokhov @ 2026-05-29 21:34 UTC (permalink / raw)
To: Jinmo Yang; +Cc: linux-input, jikos, benjamin.tissoires, stable
In-Reply-To: <20260528175945.2987781-1-jinmo44.yang@gmail.com>
On Fri, May 29, 2026 at 02:59:45AM +0900, Jinmo Yang wrote:
> wacom_wac_queue_insert() calls kfifo_skip() in a loop when the kfifo
> doesn't have enough space for the incoming report. If the kfifo is
> empty, kfifo_skip() reads stale data left in the kmalloc'd buffer
> via __kfifo_peek_n() and interprets it as a record length, advancing
> fifo->out by that garbage value. This corrupts the internal kfifo
> state, causing kfifo_unused() to return a value much larger than the
> actual buffer size, which bypasses __kfifo_in_r()'s guard:
>
> if (len + recsize > kfifo_unused(fifo))
> return 0;
>
> kfifo_copy_in() then performs an out-of-bounds memcpy, writing up to
> 3842 bytes past the 256-byte buffer.
>
> Add a !kfifo_is_empty() condition to the while loop so kfifo_skip()
> is never called on an empty fifo, and check the return value of
> kfifo_in() to reject reports that are too large for the fifo.
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Fixes: 5e013ad20689 ("HID: wacom: Remove static WACOM_PKGLEN_MAX limit")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> Changes in v2:
> - Instead of a size check at the top, add !kfifo_is_empty() to the
> while loop condition to prevent kfifo_skip() on an empty fifo
> (Suggested by Dmitry Torokhov)
> - Check kfifo_in() return value to reject oversized reports instead
> of a separate guard
>
> drivers/hid/wacom_sys.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a32320b35..489ca68f1 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -54,7 +54,7 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> {
> bool warned = false;
>
> - while (kfifo_avail(fifo) < size) {
> + while (kfifo_avail(fifo) < size && !kfifo_is_empty(fifo)) {
> if (!warned)
> hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
> warned = true;
> @@ -62,7 +62,9 @@ static void wacom_wac_queue_insert(struct hid_device *hdev,
> kfifo_skip(fifo);
> }
>
> - kfifo_in(fifo, raw_data, size);
> + if (!kfifo_in(fifo, raw_data, size))
> + hid_warn_ratelimited(hdev, "%s: report is too large (%d)\n",
> + __func__, size);
> }
>
> static void wacom_wac_queue_flush(struct hid_device *hdev,
Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2] HID: wacom: fix NULL pointer dereference in wacom_intuos_pad()
From: Dmitry Torokhov @ 2026-05-29 21:44 UTC (permalink / raw)
To: Jinmo Yang
Cc: Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
linux-input, linux-kernel
In-Reply-To: <20260523150619.615565-1-jinmo44.yang@gmail.com>
On Sun, May 24, 2026 at 12:06:19AM +0900, Jinmo Yang wrote:
> wacom_intuos_pad() dereferences wacom->pad_input without a NULL check.
> When a Wacom tablet is created via UHID with parameters that route
> through wacom_bpt_irq() -> wacom_intuos_irq() -> wacom_intuos_pad(),
> but probe did not allocate pad_input, the call to
> wacom_report_numbered_buttons() passes a NULL input_dev, causing a
> general protection fault in input_get_drvdata().
>
> Add a NULL check for pad_input at the top of wacom_intuos_pad() to
> bail out early when the pad input device was not set up.
>
> The bug was found by syzkaller on linux-next 7.1.0-rc4 (next-20260522):
>
> Oops: general protection fault, probably for non-canonical address 0xdffffc0000000052
> KASAN: null-ptr-deref in range [0x0000000000000290-0x0000000000000297]
> RIP: 0010:input_get_drvdata include/linux/input.h:390 [inline]
> RIP: 0010:wacom_report_numbered_buttons+0x37/0x210 drivers/hid/wacom_wac.c:4210
> Call Trace:
> wacom_intuos_pad drivers/hid/wacom_wac.c:643 [inline]
> wacom_intuos_irq+0x29a/0x32b0 drivers/hid/wacom_wac.c:1042
> wacom_bpt_irq drivers/hid/wacom_wac.c:3275 [inline]
> wacom_wac_irq+0x12ed/0xaef0 drivers/hid/wacom_wac.c:3545
> wacom_raw_event+0x677/0xa90 drivers/hid/wacom_sys.c:184
> __hid_input_report.constprop.0+0x39a/0x4d0 drivers/hid/hid-core.c:2161
> uhid_dev_input2 drivers/hid/uhid.c:618 [inline]
> uhid_char_write+0xa8a/0xfa0 drivers/hid/uhid.c:776
>
> Also confirmed on a Pixel 9 Pro (Android 16, kernel 6.1.124) where
> it causes an immediate kernel panic and reboot via /dev/uhid without
> requiring root privileges:
>
> KP: Oops: Fatal exception: comm:wacom_27qhdt
> Reboot reason: 0xbaba - Kernel PANIC
>
> Reproducer (unprivileged):
> open("/dev/uhid", O_RDWR)
> write(fd, UHID_CREATE2{vendor=0x056a, product=0x0020})
> write(fd, UHID_INPUT2{report_id=0x0c, size=10})
>
> Fixes: c7f0522a1ad1 ("HID: wacom: Slim down wacom_intuos_pad processing")
> Signed-off-by: Jinmo Yang <jinmo44.yang@gmail.com>
> ---
> drivers/hid/wacom_wac.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index da1f0ea85..251ddda3e 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -515,7 +515,6 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> struct wacom_features *features = &wacom->features;
> unsigned char *data = wacom->data;
> struct input_dev *input = wacom->pad_input;
> - int i;
> int buttons = 0, nbuttons = features->numbered_buttons;
> int keys = 0, nkeys = 0;
> int ring1 = 0, ring2 = 0;
> @@ -523,6 +522,10 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
> bool prox = false;
> bool wrench = false, keyboard = false, mute_touch = false, menu = false,
> info = false;
> + int i;
> +
> + if (!input)
> + return 0;
>
> /* pad packets. Works as a second tool and is always in prox */
> if (!(data[0] == WACOM_REPORT_INTUOSPAD || data[0] == WACOM_REPORT_INTUOS5PAD ||
I think there are many more places in the driver where it used
wacom->pad_input without verifying that it exists.
Thanks.
--
Dmitry
^ permalink raw reply
* [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: d3z-the-dev @ 2026-05-29 22:22 UTC (permalink / raw)
To: Even Xu, Xinpeng Sun, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, Abhishek Tamboli, Sakari Ailus,
d3z-the-dev
On the Surface Pro 10 (Meteor Lake) the touchscreen stops working after a
suspend/resume cycle and only recovers after a reboot. The driver logs
"GET_DEVICE_INFO: recv failed: -11" on resume.
The touch IC loses power during system suspend (s2idle) on this platform,
the same way it does across hibernation. quickspi_resume() only restores
the THC port, interrupts and DMA and sends a HIDSPI_ON command, assuming
the touch IC kept its power and state. When it has actually lost power the
HIDSPI_ON command is never acknowledged and the descriptor read fails,
leaving the touchscreen dead until the module is reloaded.
quickspi_restore() already handles this for hibernation by running
reset_tic() and reconfiguring the THC SPI/LTR settings. Make
quickspi_resume() do the same: quiesce interrupts, re-select the THC port,
reconfigure the SPI input/output addresses and read/write parameters, run
reset_tic() to re-enumerate the device and restore the LTR configuration.
Tested on a Surface Pro 10 across multiple s2idle suspend/resume cycles.
Link: https://github.com/linux-surface/linux-surface/issues/1799
Signed-off-by: d3z-the-dev <d3z.the.dev@gmail.com>
---
.../intel-quickspi/pci-quickspi.c | 38 +++++++++++++++++--
1 file changed, 34 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
index f669235f1883..d59278a3e5c1 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
if (!qsdev)
return -ENODEV;
+ ret = thc_interrupt_quiesce(qsdev->thc_hw, true);
+ if (ret)
+ return ret;
+
ret = thc_port_select(qsdev->thc_hw, THC_PORT_TYPE_SPI);
if (ret)
return ret;
+ thc_spi_input_output_address_config(qsdev->thc_hw,
+ qsdev->input_report_hdr_addr,
+ qsdev->input_report_bdy_addr,
+ qsdev->output_report_addr);
+
+ ret = thc_spi_read_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_read_io_mode,
+ qsdev->spi_read_opcode,
+ qsdev->spi_packet_size);
+ if (ret)
+ return ret;
+
+ ret = thc_spi_write_config(qsdev->thc_hw, qsdev->spi_freq_val,
+ qsdev->spi_write_io_mode,
+ qsdev->spi_write_opcode,
+ qsdev->spi_packet_size,
+ qsdev->performance_limit);
+ if (ret)
+ return ret;
+
thc_interrupt_config(qsdev->thc_hw);
thc_interrupt_enable(qsdev->thc_hw, true);
- ret = thc_dma_configure(qsdev->thc_hw);
+ /* The TIC may lose power across system suspend, reset it to recover */
+ ret = reset_tic(qsdev);
if (ret)
return ret;
- ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
+ ret = thc_dma_configure(qsdev->thc_hw);
if (ret)
return ret;
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
+ thc_ltr_config(qsdev->thc_hw,
+ qsdev->active_ltr_val,
+ qsdev->low_power_ltr_val);
+
+ thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
+
+ qsdev->state = QUICKSPI_ENABLED;
return 0;
}
--
2.54.0
^ permalink raw reply related
* Re: [PATCH] HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
From: sashiko-bot @ 2026-05-29 23:12 UTC (permalink / raw)
To: d3z-the-dev; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260529222210.1532876-1-d3z.the.dev@gmail.com>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Unconditional hardware reset during system resume destroys pending wake events and breaks Wake-on-Touch.
Pre-existing issues:
- [High] Using wait_event_interruptible_timeout in a system resume callback can prematurely abort device resume.
- [High] Concurrent execution of `reset_tic()` from the PM resume callback and the IRQ thread without proper locking.
--
commit e9088cb0484bc3fbc7b5dd49968293dc14d55d36
Author: d3z-the-dev <d3z.the.dev@gmail.com>
HID: intel-thc-hid: intel-quickspi: reset touch IC on system resume
This commit modifies quickspi_resume() to perform a full reset of the touch
IC during system resume. It mirrors the hibernation restore path to fix an
issue where the touch IC loses power during s2idle suspend on certain platforms.
Link: https://github.com/linux-surface/linux-surface/issues/1799
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index f669235f18839..d59278a3e5c10 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
[ ... ]
> @@ -780,24 +780,54 @@ static int quickspi_resume(struct device *device)
> thc_interrupt_config(qsdev->thc_hw);
>
> thc_interrupt_enable(qsdev->thc_hw, true);
[Severity: High]
This is a pre-existing issue, but does enabling interrupts right before calling
reset_tic() introduce a race condition?
If the touch controller generates an error interrupt during the reset sequence,
the OS schedules the IRQ thread, which calls try_recover() and executes
reset_tic() again. Since both the PM thread and the IRQ thread execute
reset_tic() concurrently without a mutex:
PM thread
quickspi_resume()
reset_tic()
IRQ thread
quickspi_irq_thread_handler()
try_recover()
reset_tic()
Can this corrupt the device state and cause both threads to wait on the same
reset_ack_wq?
>
> - ret = thc_dma_configure(qsdev->thc_hw);
> + /* The TIC may lose power across system suspend, reset it to recover */
> + ret = reset_tic(qsdev);
[Severity: High]
This isn't a bug introduced by this patch, but since reset_tic() is now
used in the primary system resume path, is it safe for it to use
wait_event_interruptible_timeout() internally?
System resume callbacks are often executed by a user-space process which
might have a pending signal from the task freeze phase. If
wait_event_interruptible_timeout() aborts immediately and returns
-ERESTARTSYS, won't reset_tic() fail with -ETIMEDOUT and completely abort
the device resume?
> if (ret)
> return ret;
>
> - ret = thc_interrupt_quiesce(qsdev->thc_hw, false);
> + ret = thc_dma_configure(qsdev->thc_hw);
> if (ret)
> return ret;
>
> - if (!device_may_wakeup(qsdev->dev))
> - return quickspi_set_power(qsdev, HIDSPI_ON);
[Severity: High]
Does unconditionally calling reset_tic() here break Wake-on-Touch for
platforms that don't lose power?
Previously, this function had a check to avoid disturbing a wake-enabled
device:
- if (!device_may_wakeup(qsdev->dev))
- return quickspi_set_power(qsdev, HIDSPI_ON);
Will a hard reset wipe the internal buffers of the touch controller before
the OS can fetch the wake-triggering touch event, resulting in permanently
lost events?
> + thc_ltr_config(qsdev->thc_hw,
> + qsdev->active_ltr_val,
> + qsdev->low_power_ltr_val);
> +
> + thc_change_ltr_mode(qsdev->thc_hw, THC_LTR_MODE_ACTIVE);
> +
> + qsdev->state = QUICKSPI_ENABLED;
>
> return 0;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529222210.1532876-1-d3z.the.dev@gmail.com?part=1
^ permalink raw reply
* Re: [PATCH 05/10] [v2] mips: select legacy gpiolib interfaces where used
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-6-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] Duplicate `select GPIOLIB` in `config MIPS_ALCHEMY`.
- [Low] Unnecessary `select GPIOLIB_LEGACY` for `config MIKROTIK_RB532`.
--
commit 0726793f1a39f1a0b2ac5d24243bcbe5e8999b93
Author: Arnd Bergmann <arnd@arndb.de>
mips: select legacy gpiolib interfaces where used
This commit updates several old mips machines that have not been converted
away from the old-style gpiolib interfaces to select CONFIG_GPIOLIB_LEGACY.
This ensures the legacy code continues to work where needed but can be
omitted otherwise.
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 4364f3dba688f..b91e62d69a5db 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -206,6 +206,8 @@ config MIPS_ALCHEMY
> select CSRC_R4K
> select IRQ_MIPS_CPU
> select DMA_NONCOHERENT # Au1000,1500,1100 aren't, rest is
> + select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Is this select GPIOLIB a duplicate? It appears select GPIOLIB is already
present further down in the MIPS_ALCHEMY block.
> select MIPS_FIXUP_BIGPHYS_ADDR if PCI
> select SYS_HAS_CPU_MIPS32_R1
> select SYS_SUPPORTS_32BIT_KERNEL
[ ... ]
> @@ -999,6 +1003,7 @@ config MIKROTIK_RB532
> select SWAP_IO_SPACE
> select BOOT_RAW
> select GPIOLIB
> + select GPIOLIB_LEGACY
[Severity: Low]
Does this select GPIOLIB_LEGACY introduce an unnecessary dependency for
MIKROTIK_RB532? A look at the code indicates that MIKROTIK_RB532 and its
associated drivers (like pata_rb532_cf and rb532_button) use the modern
descriptor-based gpiod API and do not seem to call the legacy GPIO functions.
Could this dependency be omitted so we don't compile legacy interfaces
unnecessarily?
> select MIPS_L1_CACHE_SHIFT_4
> help
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=5
^ permalink raw reply
* Re: [PATCH 08/10] [v6 net-next] p54spi: convert to devicetree
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-9-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] SPI transfers are using stack-allocated buffers for `tx_buf` and `rx_buf`, violating the SPI subsystem's DMA requirements.
--
commit 8ba2b2da7516b867a52706d993b9ce2d49c99105
Author: Arnd Bergmann <arnd@arndb.de>
p54spi: convert to devicetree
The Prism54 SPI driver hardcodes GPIO numbers and expects users to
pass them as module parameters, apparently a relic from its life as a
staging driver. This works because there is only one user, the Nokia
N8x0 tablet.
Convert this to the gpio descriptor interface and DT based probing
to improve this and simplify the code at the same time.
> diff --git a/drivers/net/wireless/intersil/p54/p54spi.c b/drivers/net/wireless/intersil/p54/p54spi.c
> index 9d66dcae54e02..d18be25450285 100644
> --- a/drivers/net/wireless/intersil/p54/p54spi.c
> +++ b/drivers/net/wireless/intersil/p54/p54spi.c
[ ... ]
> @@ -30,19 +31,6 @@
> MODULE_FIRMWARE("3826.arm");
> MODULE_FIRMWARE("3826.eeprom");
>
> -/* gpios should be handled in board files and provided via platform data,
> - * but because it's currently impossible for p54spi to have a header file
> - * in include/linux, let's use module parameters for now
> - */
> -
> -static int p54spi_gpio_power = 97;
> -module_param(p54spi_gpio_power, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_power, "gpio number for power line");
> -
> -static int p54spi_gpio_irq = 87;
> -module_param(p54spi_gpio_irq, int, 0444);
> -MODULE_PARM_DESC(p54spi_gpio_irq, "gpio number for irq line");
> -
> static void p54spi_spi_read(struct p54s_priv *priv, u8 address,
> void *buf, size_t len)
> {
[Severity: High]
This isn't a bug introduced by this patch, but does this code use
stack-allocated buffers for SPI transfers?
It appears that p54spi_spi_read() and other similar functions pass stack
variables (e.g., addr in p54spi_spi_read(), rx_head in p54spi_rx(), and
val in p54spi_read32()) directly into spi_sync() as tx_buf and rx_buf.
If the underlying SPI controller uses DMA, the SPI core maps these buffers.
Using stack memory for DMA is unsafe because, with options like
CONFIG_VMAP_STACK enabled, the stack may not be physically contiguous.
Even without it, stack DMA leads to cache-line sharing and incoherency,
which can result in severe memory corruption or kernel panics.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=8
^ permalink raw reply
* Re: [PATCH 10/10] gpiolib: turn off legacy interface by default
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-11-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Turning off `CONFIG_GPIOLIB_LEGACY` by default will break the build for multiple architectures and platforms that still rely on the legacy GPIO interface but do not select `CONFIG_GPIOLIB_LEGACY`.
--
commit a920fbf61a1d127c3a98b41cb32f7260acd5657b
Author: Arnd Bergmann <arnd@arndb.de>
gpiolib: turn off legacy interface by default
This commit removes the default enablement of GPIOLIB_LEGACY, requiring
platforms to explicitly select it if they still use the legacy interface.
It also adds a prompt to enable it manually for compile testing.
> All users of the legacy interface now select CONFIG_GPIOLIB_LEGACY,
> so it can be turned off by default and only get built on platforms
> that still have one unconverted driver.
[Severity: Low]
Is this statement accurate? There appear to be platforms like ARM PXA
(arch/arm/mach-pxa/spitz_pm.c), ARM Orion5x, and m68k Coldfire that still
contain direct calls to legacy GPIO functions like gpio_request() but do
not select GPIOLIB_LEGACY in their Kconfig files.
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 020e51e30317a..543de15a12f63 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -4,7 +4,14 @@
> #
>
> config GPIOLIB_LEGACY
> - def_bool y
[Severity: Low]
Will removing this default cause build regressions for the unconverted
platforms mentioned above?
It seems the legacy function declarations in include/linux/gpio.h will be
hidden, leading to implicit declaration errors when compiling those
architectures.
> + bool "Legacy GPIO interfaces" if COMPILE_TEST
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=10
^ permalink raw reply
* Re: [PATCH 02/10] [v3] input: gpio-keys: make legacy gpiolib optional
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260520183815.2510387-3-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying the legacy GPIO validation to check `!button->irq` breaks legacy devices that legitimately provide both a GPIO and an IRQ.
- [Medium] Conditionally removing the `gpio` field from `struct gpio_keys_button` causes build failures in drivers that assign to this field when `CONFIG_GPIOLIB_LEGACY` is disabled.
--
commit cb399daff5ceabcf54d5ea611fd309f5c0600ab4
Author: Arnd Bergmann <arnd@arndb.de>
input: gpio-keys: make legacy gpiolib optional
This commit makes the legacy gpiolib interface optional for gpio-keys and
gpio-keys-polled by adding ifdef blocks. It also modifies the gpio-keys
driver to ignore the gpio number if an IRQ is passed to keep Rohm PMIC
drivers working with and without CONFIG_GPIOLIB_LEGACY.
Link: https://lore.kernel.org/all/b3c94552-c104-42e3-be15-7e8362e8039e@gmail.com/
Link: https://lore.kernel.org/all/afJXG4_rtaj3l2Dk@google.com/
> diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
[ ... ]
> @@ -528,7 +528,8 @@ static int gpio_keys_setup_key(struct platform_device *pdev,
> */
> bdata->gpiod = NULL;
> }
> - } else if (gpio_is_valid(button->gpio)) {
> +#ifdef CONFIG_GPIOLIB_LEGACY
> + } else if (!button->irq && gpio_is_valid(button->gpio)) {
[Severity: High]
Does this change break legacy devices that legitimately provide both a valid
GPIO and a valid IRQ?
If a legacy platform provides both a valid GPIO and an IRQ, this condition
now evaluates to false.
By skipping the GPIO request entirely, the driver falls back to treating the
device as a pure IRQ button. This forces the driver to lose hardware
debouncing and rely on a simulated release timer instead of checking the
physical GPIO line.
[ ... ]
> diff --git a/include/linux/gpio_keys.h b/include/linux/gpio_keys.h
> --- a/include/linux/gpio_keys.h
> +++ b/include/linux/gpio_keys.h
[ ... ]
> @@ -25,7 +25,9 @@ struct device;
> */
> struct gpio_keys_button {
> unsigned int code;
> +#ifdef CONFIG_GPIOLIB_LEGACY
> int gpio;
> +#endif
> int active_low;
[Severity: Medium]
Will this cause build failures for drivers that assign to this field when
CONFIG_GPIOLIB_LEGACY is disabled?
Other modern drivers dynamically create the platform device and pass legacy
GPIO numbers by directly assigning to this field:
drivers/input/misc/soc_button_array.c:soc_button_device_create() {
...
gpio_keys[n_buttons].gpio = gpio;
...
}
When CONFIG_GPIOLIB_LEGACY is disabled, this results in a compilation failure
since struct gpio_keys_button no longer has the gpio member.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=2
^ permalink raw reply
* Re: [PATCH 01/10] [v2] [net-next] net: dsa: b53: hide legacy gpiolib usage on non-mips
From: sashiko-bot @ 2026-05-30 0:45 UTC (permalink / raw)
To: Arnd Bergmann; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260520183815.2510387-2-arnd@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Probe errors from devm_gpio_request_one() are silently ignored, failing to propagate critical errors such as -EPROBE_DEFER.
--
commit 46ca85c6cb9da93cad093c8eb342afc0ea94be33
Author: Arnd Bergmann <arnd@arndb.de>
net: dsa: b53: hide legacy gpiolib usage on non-mips
This commit moves legacy gpiolib usage inside an #ifdef block and transitions
the common parts of the driver to use modern GPIO descriptors. This allows
building the driver even when gpio_set_value is left out of the kernel.
> diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
> index 2c4131ed7e30b..d838511ae6744 100644
> --- a/drivers/net/dsa/b53/b53_common.c
> +++ b/drivers/net/dsa/b53/b53_common.c
[ ... ]
> @@ -3173,12 +3172,6 @@ static int b53_switch_init(struct b53_device *dev)
> return -ENOMEM;
>
> dev->reset_gpio = b53_switch_get_reset_gpio(dev);
> - if (dev->reset_gpio >= 0) {
> - ret = devm_gpio_request_one(dev->dev, dev->reset_gpio,
> - GPIOF_OUT_INIT_HIGH, "robo_reset");
> - if (ret)
> - return ret;
> - }
>
> return 0;
> }
[Severity: High]
Does this silently ignore probe errors like -EPROBE_DEFER?
Previously, if devm_gpio_request_one failed, the error was returned to the
caller. Now, b53_switch_get_reset_gpio handles the request and returns an
ERR_PTR on failure, but that error pointer is simply stored in dev->reset_gpio
while b53_switch_init unconditionally returns 0.
Later in the code, b53_switch_reset_gpio relies on IS_ERR(dev->reset_gpio)
to gracefully return, which would silently skip the reset sequence entirely
if the GPIO controller simply isn't ready yet (-EPROBE_DEFER) or if there
was a memory allocation failure (-ENOMEM).
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260520183815.2510387-1-arnd@kernel.org?part=1
^ permalink raw reply
* Re: [PATCH 4/4] dt-bindings: input: remove obsolete matrix-keymap.txt
From: sashiko-bot @ 2026-05-30 5:23 UTC (permalink / raw)
To: Akash Sukhavasi; +Cc: dmitry.torokhov, robh, linux-input, devicetree, conor+dt
In-Reply-To: <20260529052246.4934-5-akash.sukhavasi@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The commit message inaccurately claims that 'Eight files still reference matrix-keymap.txt', when only 6 files actually do.
--
commit eb5de2cbf16470ef87ea36fcd4695758a1c30a9f
Author: Akash Sukhavasi <akash.sukhavasi@gmail.com>
dt-bindings: input: remove obsolete matrix-keymap.txt
This commit removes the obsolete matrix-keymap.txt stub and updates all
remaining references across the input and mfd binding documentation to
point directly to the matrix-keymap.yaml schema.
> Eight files still reference matrix-keymap.txt, forcing readers
> through an extra hop to reach the .yaml. The stub has not been
> touched since June 2020. Update all references across input and mfd
> binding documentation to point directly to matrix-keymap.yaml and
> remove the stub.
[Severity: Low]
This isn't a bug, but the commit message states that eight files still
reference the text file. Looking at the patch, there are eight total
references updated, but they are spread across only six files since
tc3589x.txt contains three of those references.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260529052246.4934-1-akash.sukhavasi@gmail.com?part=4
^ permalink raw reply
* [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface
From: Dmitry Torokhov @ 2026-05-30 5:40 UTC (permalink / raw)
To: linux-input; +Cc: linux-kernel
When the driver was converted to use the driver core to instantiate device
attributes (via .dev_groups in the usb_driver structure), the attributes
started appearing on all interfaces bound to the driver. Since the ims-pcu
driver manually claims the secondary data interface during probe, the
driver core automatically creates the sysfs attributes for that interface
as well.
However, the driver only supports these attributes on the primary control
interface. Data interfaces lack the necessary descriptors and internal
state to handle these requests, and accessing them can lead to unexpected
behavior or crashes.
Fix this by updating the is_visible() callbacks for both the main and OFN
attribute groups to verify that the interface being accessed is indeed the
control interface.
Fixes: 204d18a7a0c6 ("Input: ims-pcu - use driver core to instantiate device attributes")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/ims-pcu.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 13367ec66936..ddfb88b7c8f5 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1283,6 +1283,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
if (pcu->bootloader_mode) {
if (attr != &dev_attr_update_firmware_status.attr &&
attr != &dev_attr_update_firmware.attr &&
@@ -1534,6 +1537,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
struct ims_pcu *pcu = usb_get_intfdata(intf);
umode_t mode = attr->mode;
+ if (intf != pcu->ctrl_intf)
+ return 0;
+
/*
* PCU-B devices, both GEN_1 and GEN_2 do not have OFN sensor.
*/
--
2.54.0.823.g6e5bcc1fc9-goog
--
Dmitry
^ permalink raw reply related
* [PATCH] Input: atlas_btns - modernize the driver
From: Dmitry Torokhov @ 2026-05-30 5:52 UTC (permalink / raw)
To: linux-input; +Cc: Rafael J. Wysocki, linux-kernel
Rework the driver to use modern style:
- Remove global state by introducing a per-device structure
- Use devm for resource management (input device allocation)
- Use dev_err_probe() for error reporting in the probe path
- Clean up unused definitions and headers.
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/misc/atlas_btns.c | 109 ++++++++++++++++----------------
1 file changed, 55 insertions(+), 54 deletions(-)
diff --git a/drivers/input/misc/atlas_btns.c b/drivers/input/misc/atlas_btns.c
index 835ad45a9d65..5805a1a3bb05 100644
--- a/drivers/input/misc/atlas_btns.c
+++ b/drivers/input/misc/atlas_btns.c
@@ -15,17 +15,18 @@
#include <linux/types.h>
#include <linux/acpi.h>
#include <linux/platform_device.h>
-#include <linux/uaccess.h>
#define ACPI_ATLAS_NAME "Atlas ACPI"
-#define ACPI_ATLAS_CLASS "Atlas"
-static unsigned short atlas_keymap[16];
-static struct input_dev *input_dev;
+struct atlas_btns {
+ struct input_dev *input_dev;
+ unsigned short keymap[16];
+};
/* button handling code */
static acpi_status acpi_atlas_button_setup(acpi_handle region_handle,
- u32 function, void *handler_context, void **return_context)
+ u32 function, void *handler_context,
+ void **return_context)
{
*return_context =
(function != ACPI_REGION_DEACTIVATE) ? handler_context : NULL;
@@ -34,33 +35,37 @@ static acpi_status acpi_atlas_button_setup(acpi_handle region_handle,
}
static acpi_status acpi_atlas_button_handler(u32 function,
- acpi_physical_address address,
- u32 bit_width, u64 *value,
- void *handler_context, void *region_context)
+ acpi_physical_address address,
+ u32 bit_width, u64 *value,
+ void *handler_context,
+ void *region_context)
{
- acpi_status status;
+ struct atlas_btns *atlas = region_context;
if (function == ACPI_WRITE) {
int code = address & 0x0f;
int key_down = !(address & 0x10);
- input_event(input_dev, EV_MSC, MSC_SCAN, code);
- input_report_key(input_dev, atlas_keymap[code], key_down);
- input_sync(input_dev);
+ input_event(atlas->input_dev, EV_MSC, MSC_SCAN, code);
+ input_report_key(atlas->input_dev, atlas->keymap[code],
+ key_down);
+ input_sync(atlas->input_dev);
- status = AE_OK;
- } else {
- pr_warn("shrugged on unexpected function: function=%x,address=%lx,value=%x\n",
- function, (unsigned long)address, (u32)*value);
- status = AE_BAD_PARAMETER;
+ return AE_OK;
}
- return status;
+ dev_warn(atlas->input_dev->dev.parent,
+ "unexpected function: function=%x,address=%lx,value=%x\n",
+ function, (unsigned long)address, (u32)*value);
+
+ return AE_BAD_PARAMETER;
}
static int atlas_acpi_button_probe(struct platform_device *pdev)
{
struct acpi_device *device;
+ struct atlas_btns *atlas;
+ struct input_dev *input_dev;
acpi_status status;
int i;
int err;
@@ -69,65 +74,62 @@ static int atlas_acpi_button_probe(struct platform_device *pdev)
if (!device)
return -ENODEV;
- input_dev = input_allocate_device();
- if (!input_dev) {
- pr_err("unable to allocate input device\n");
+ atlas = devm_kzalloc(&pdev->dev, sizeof(*atlas), GFP_KERNEL);
+ if (!atlas)
return -ENOMEM;
- }
+
+ input_dev = devm_input_allocate_device(&pdev->dev);
+ if (!input_dev)
+ return -ENOMEM;
+
+ atlas->input_dev = input_dev;
input_dev->name = "Atlas ACPI button driver";
input_dev->phys = "ASIM0000/atlas/input0";
input_dev->id.bustype = BUS_HOST;
- input_dev->keycode = atlas_keymap;
- input_dev->keycodesize = sizeof(unsigned short);
- input_dev->keycodemax = ARRAY_SIZE(atlas_keymap);
+ input_dev->keycode = atlas->keymap;
+ input_dev->keycodesize = sizeof(atlas->keymap[0]);
+ input_dev->keycodemax = ARRAY_SIZE(atlas->keymap);
input_set_capability(input_dev, EV_MSC, MSC_SCAN);
- __set_bit(EV_KEY, input_dev->evbit);
- for (i = 0; i < ARRAY_SIZE(atlas_keymap); i++) {
+ for (i = 0; i < ARRAY_SIZE(atlas->keymap); i++) {
if (i < 9) {
- atlas_keymap[i] = KEY_F1 + i;
- __set_bit(KEY_F1 + i, input_dev->keybit);
- } else
- atlas_keymap[i] = KEY_RESERVED;
+ atlas->keymap[i] = KEY_F1 + i;
+ input_set_capability(input_dev, EV_KEY, KEY_F1 + i);
+ } else {
+ atlas->keymap[i] = KEY_RESERVED;
+ }
}
err = input_register_device(input_dev);
- if (err) {
- pr_err("couldn't register input device\n");
- input_free_device(input_dev);
- return err;
- }
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "couldn't register input device\n");
/* hookup button handler */
status = acpi_install_address_space_handler(device->handle,
- 0x81, &acpi_atlas_button_handler,
- &acpi_atlas_button_setup, device);
- if (ACPI_FAILURE(status)) {
- pr_err("error installing addr spc handler\n");
- input_unregister_device(input_dev);
- err = -EINVAL;
- }
+ 0x81,
+ &acpi_atlas_button_handler,
+ &acpi_atlas_button_setup,
+ atlas);
+ if (ACPI_FAILURE(status))
+ return dev_err_probe(&pdev->dev, -EINVAL,
+ "error installing addr spc handler\n");
- return err;
+ return 0;
}
static void atlas_acpi_button_remove(struct platform_device *pdev)
{
struct acpi_device *device = ACPI_COMPANION(&pdev->dev);
- acpi_status status;
- status = acpi_remove_address_space_handler(device->handle,
- 0x81, &acpi_atlas_button_handler);
- if (ACPI_FAILURE(status))
- pr_err("error removing addr spc handler\n");
-
- input_unregister_device(input_dev);
+ acpi_remove_address_space_handler(device->handle, 0x81,
+ &acpi_atlas_button_handler);
}
static const struct acpi_device_id atlas_device_ids[] = {
- {"ASIM0000", 0},
- {"", 0},
+ { "ASIM0000" },
+ { "" }
};
MODULE_DEVICE_TABLE(acpi, atlas_device_ids);
@@ -144,4 +146,3 @@ module_platform_driver(atlas_acpi_driver);
MODULE_AUTHOR("Jaya Kumar");
MODULE_LICENSE("GPL");
MODULE_DESCRIPTION("Atlas button driver");
-
--
2.54.0.823.g6e5bcc1fc9-goog
--
Dmitry
^ permalink raw reply related
* Re: [PATCH] Input: ims-pcu - only expose sysfs attributes on control interface
From: sashiko-bot @ 2026-05-30 5:53 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <ahp23lj4_vXIeUBl@google.com>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [Low] The `ims_pcu_ofn_is_attr_visible()` function introduced in the patch returns `0` when `intf != pcu->ctrl_intf` for the `ofn` attribute group. Because `ofn` is a named attribute group, returning `0` suppresses the individual files but still creates an empty 'ofn' directory on the data interface.
Pre-existing issues:
- [High] The `buttons->input` pointer is assigned after `input_register_device()`, and there is no memory barrier before setting `pcu->setup_complete = true`. On weakly ordered architectures, the CPU can reorder these stores, leading to a NULL pointer dereference in the interrupt handler (`ims_pcu_handle_response`) if an event arrives and observes `setup_complete == true` before `buttons->input` is initialized.
- [High] The driver claims the secondary data interface (`pcu->data_intf`) in `ims_pcu_probe` but fails to release it in `ims_pcu_disconnect` when unbinding the control interface. The data interface remains bound with its `intfdata` pointing to the freed `pcu` structure. Subsequent sysfs accesses on the data interface will dereference this freed pointer, resulting in a use-after-free (UAF).
- [High] A malicious USB device can provide an incorrect master interface number in the CDC Union descriptor. `ims_pcu_parse_cdc_data()` uses this to set `pcu->ctrl_intf` and assigns `intfdata` to it. If this differs from the interface currently being probed, the probed interface will have `NULL` driver data. When the sysfs core subsequently calls `ims_pcu_is_attr_visible()`, `usb_get_intfdata()` returns `NULL`, causing a NULL pointer dereference when accessing `pcu->ctrl_intf`.
- [High] The sysfs attribute store function `ims_pcu_reset_device` calls `ims_pcu_execute_command()` without acquiring `pcu->cmd_mutex`. This allows concurrent sysfs operations (e.g., setting backlight brightness or updating firmware) to interleave their command preparations, causing data corruption in the shared `pcu->urb_out_buf` and race conditions on the `pcu->cmd_done` completion structure.
- [High] In `ims_pcu_buffers_free()`, the input URB DMA buffer (`pcu->urb_in_buf`) is incorrectly freed using `pcu->max_out_size` instead of the `pcu->max_in_size` used during allocation.
--
commit e7e143a0135bed02c30cf00e17a9a25adf1ab6c4
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: ims-pcu - only expose sysfs attributes on control interface
When the driver was converted to use the driver core to instantiate device
attributes, they started appearing on all interfaces bound to the driver.
This patch updates the is_visible callbacks to verify that the interface
being accessed is indeed the control interface.
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..7c76bc82ab703 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1249,6 +1249,9 @@ static umode_t ims_pcu_is_attr_visible(struct kobject *kobj,
> struct ims_pcu *pcu = usb_get_intfdata(intf);
> umode_t mode = attr->mode;
>
> + if (intf != pcu->ctrl_intf)
> + return 0;
[Severity: High]
This isn't a bug introduced by this patch, but could pcu be NULL here?
If a malicious USB device provides an incorrect master interface number in the
CDC Union descriptor, ims_pcu_parse_cdc_data() might set pcu->ctrl_intf to
a different interface than the one being probed. When the sysfs core
subsequently calls ims_pcu_is_attr_visible(), usb_get_intfdata() would
return NULL, leading to a NULL pointer dereference when accessing
pcu->ctrl_intf.
[ ... ]
> @@ -1488,6 +1491,9 @@ static umode_t ims_pcu_ofn_is_attr_visible(struct kobject *kobj,
> struct ims_pcu *pcu = usb_get_intfdata(intf);
> umode_t mode = attr->mode;
>
> + if (intf != pcu->ctrl_intf)
> + return 0;
[Severity: Low]
Since ofn is a named attribute group, does returning 0 here suppress the
individual files but still leave an empty ofn directory on the data interface?
According to sysfs guidelines, should this return SYSFS_GROUP_INVISIBLE
instead to fully suppress the group directory?
I also noticed a few pre-existing issues in the surrounding code:
drivers/input/misc/ims-pcu.c:ims_pcu_init_application_mode() {
...
if (info->has_gamepad) {
error = ims_pcu_setup_gamepad(pcu);
if (error)
goto err_destroy_buttons;
}
pcu->setup_complete = true;
return 0;
}
[Severity: High]
This is a pre-existing issue, but should there be a memory barrier before
setting pcu->setup_complete = true?
On weakly ordered architectures, could the CPU reorder these stores? If an
event arrives and ims_pcu_handle_response() observes setup_complete == true
before buttons->input is fully initialized, it might lead to a NULL pointer
dereference in the interrupt handler.
drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
...
/*
* See if we are dealing with control or data interface. The cleanup
* happens when we unbind primary (control) interface.
*/
if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
return;
ims_pcu_stop_io(pcu);
...
ims_pcu_buffers_free(pcu);
kfree(pcu);
}
[Severity: High]
This isn't a bug introduced by this patch, but does this fail to release the
claimed data interface?
If the driver claims the secondary data interface (pcu->data_intf) in
ims_pcu_probe(), failing to release it via usb_driver_release_interface()
when unbinding the control interface results in an asymmetric teardown.
The data interface remains bound with its intfdata pointing to the freed
pcu structure, and reading its sysfs attributes could trigger a use-after-free.
drivers/input/misc/ims-pcu.c:ims_pcu_reset_device() {
...
dev_info(pcu->dev, "Attempting to reset device\n");
error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
if (error) {
...
}
[Severity: High]
This is a pre-existing issue, but shouldn't ims_pcu_reset_device() acquire
pcu->cmd_mutex before calling ims_pcu_execute_command()?
Without acquiring the lock, concurrent sysfs operations (e.g., setting
backlight brightness or updating firmware) could interleave their command
preparations. This could cause data corruption in the shared
pcu->urb_out_buf and race conditions on the pcu->cmd_done completion
structure.
drivers/input/misc/ims-pcu.c:ims_pcu_buffers_free() {
...
usb_kill_urb(pcu->urb_in);
usb_free_urb(pcu->urb_in);
usb_free_coherent(pcu->udev, pcu->max_out_size,
pcu->urb_in_buf, pcu->read_dma);
...
}
[Severity: High]
This isn't a bug introduced by this patch, but is pcu->urb_in_buf being freed
with the wrong size?
It appears the input URB DMA buffer is incorrectly freed using
pcu->max_out_size instead of the pcu->max_in_size that was used during
allocation. Depending on the DMA allocator, this could lead to memory
corruption or leaks.
--
Sashiko AI review · https://sashiko.dev/#/patchset/ahp23lj4_vXIeUBl@google.com?part=1
^ permalink raw reply
* Re: [PATCH v1] Input: Use named initializers for arrays of i2c_device_data
From: Dmitry Torokhov @ 2026-05-30 5:56 UTC (permalink / raw)
To: Uwe Kleine-König (The Capable Hub)
Cc: Anshul Dalal, Michael Hennerich, Yassine Oudjana, Linus Walleij,
Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Support Opensource, Nick Dyer, Hans de Goede, Job Noorman,
Mika Penttilä, Maxime Coquelin, Alexandre Torgue, Kees Cook,
bui duc phuc, Thorsten Blum, Yauhen Kharuzhy, Sakari Ailus,
Marco Crivellari, Minseong Kim, Ingo Molnar, Thomas Gleixner,
Oleh Kuzhylnyi, Marek Vasut, Krzysztof Kozlowski,
Geert Uytterhoeven, Josua Mayer, Michael Tretter, Jeff LaBundy,
Javier Carrasco, David Heidelberg, Petr Hodina, Svyatoslav Ryhel,
Johannes Kirchmair, Andy Shevchenko, Xichao Zhao, linux-input,
linux-kernel, linux-arm-kernel, platform-driver-x86, linux-stm32
In-Reply-To: <20260515164848.497608-2-u.kleine-koenig@baylibre.com>
Hi Uwe,
On Fri, May 15, 2026 at 06:48:47PM +0200, Uwe Kleine-König (The Capable Hub) wrote:
> While being less compact, using named initializers allows to more easily
> see which members of the structs are assigned which value without having
> to lookup the declaration of the struct. And it's also more robust
> against changes to the struct definition.
>
> The mentioned robustness is relevant for a planned change to struct
> i2c_device_id that replaces .driver_data by an anonymous union.
>
> This patch doesn't modify the compiled arrays, only their representation
> in source form benefits. The former was confirmed with x86 and arm64
> builds.
>
> Signed-off-by: Uwe Kleine-König (The Capable Hub) <u.kleine-koenig@baylibre.com>
> ---
> Hello,
>
> the mentioned change to i2c_device_id is the following:
>
> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 23ff24080dfd..aebd3a5e90af 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -477,7 +477,11 @@ struct rpmsg_device_id {
>
> struct i2c_device_id {
> char name[I2C_NAME_SIZE];
> - kernel_ulong_t driver_data; /* Data private to the driver */
> + union {
> + /* Data private to the driver */
> + kernel_ulong_t driver_data;
> + const void *driver_data_ptr;
> + };
> };
>
> /* pci_epf */
>
> and this requires that .driver_data is assigned via a named initializer
> for static data. This requirement isn't a bad one because named
> initializers are also much better readable than list initializers.
>
> The union added to struct i2c_device_id enables further cleanups like:
>
> diff --git a/drivers/input/touchscreen/ili210x.c b/drivers/input/touchscreen/ili210x.c
> index 66ada7ffbc80..94aa4dc002c5 100644
> --- a/drivers/input/touchscreen/ili210x.c
> +++ b/drivers/input/touchscreen/ili210x.c
> @@ -969,7 +969,7 @@ static int ili210x_i2c_probe(struct i2c_client *client)
>
> chip = device_get_match_data(dev);
> if (!chip && id)
> - chip = (const struct ili2xxx_chip *)id->driver_data;
> + chip = id->driver_data_ptr;
> if (!chip)
> return dev_err_probe(&client->dev, -ENODEV, "unknown device model\n");
>
> @@ -1049,10 +1049,10 @@ static int ili210x_i2c_probe(struct i2c_client *client)
> }
>
> static const struct i2c_device_id ili210x_i2c_id[] = {
> - { .name = "ili210x", .driver_data = (long)&ili210x_chip },
> - { .name = "ili2117", .driver_data = (long)&ili211x_chip },
> - { .name = "ili2120", .driver_data = (long)&ili212x_chip },
> - { .name = "ili251x", .driver_data = (long)&ili251x_chip },
> + { .name = "ili210x", .driver_data_ptr = &ili210x_chip },
> + { .name = "ili2117", .driver_data_ptr = &ili211x_chip },
> + { .name = "ili2120", .driver_data_ptr = &ili212x_chip },
> + { .name = "ili251x", .driver_data_ptr = &ili251x_chip },
> { }
> };
> MODULE_DEVICE_TABLE(i2c, ili210x_i2c_id);
>
> that are an improvement for readability (again!) and it keeps some
> properties of the pointers (here: being const) without having to pay
> attention for that.
>
> My additional motivation for this effort is CHERI[1]. This is a hardware
> extension that uses 128 bit pointers but unsigned long is still 64 bit.
> So with CHERI you cannot store pointers in unsigned long variables.
I like the ability to properly set up pointers for driver data, however
I do not think we should use named initializers for name field. As long
as we are not planning on moving its position I like the brevity of just
saying
{ "ili210x", .driver_data_ptr = &ili210x_chip },
Can we keep the old style for the name field?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1] Input: Use named initializers for arrays of i2c_device_data
From: Uwe Kleine-König (The Capable Hub) @ 2026-05-30 12:02 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Anshul Dalal, Michael Hennerich, Yassine Oudjana, Linus Walleij,
Nicolas Ferre, Alexandre Belloni, Claudiu Beznea,
Support Opensource, Nick Dyer, Hans de Goede, Job Noorman,
Mika Penttilä, Maxime Coquelin, Alexandre Torgue, Kees Cook,
bui duc phuc, Thorsten Blum, Yauhen Kharuzhy, Sakari Ailus,
Marco Crivellari, Minseong Kim, Ingo Molnar, Thomas Gleixner,
Oleh Kuzhylnyi, Marek Vasut, Krzysztof Kozlowski,
Geert Uytterhoeven, Josua Mayer, Michael Tretter, Jeff LaBundy,
Javier Carrasco, David Heidelberg, Petr Hodina, Svyatoslav Ryhel,
Johannes Kirchmair, Andy Shevchenko, Xichao Zhao, linux-input,
linux-kernel, linux-arm-kernel, platform-driver-x86, linux-stm32
In-Reply-To: <ahp7WuHiPYa7Q_pX@google.com>
[-- Attachment #1: Type: text/plain, Size: 1055 bytes --]
Hello Dmitry,
On Fri, May 29, 2026 at 10:56:48PM -0700, Dmitry Torokhov wrote:
> I like the ability to properly set up pointers for driver data, however
> I do not think we should use named initializers for name field. As long
> as we are not planning on moving its position I like the brevity of just
> saying
>
> { "ili210x", .driver_data_ptr = &ili210x_chip },
>
> Can we keep the old style for the name field?
I have no strong opinion here, my patch introduces the named initializer
for both members for consistency. Note that my patches for all the other
subsystems (and also other *_device_id) do the same and you're the first
maintainer with that expressed preference.
I even sent a few patches that adapt initializers without .driver_data
(e.g.
https://lore.kernel.org/all/177932943265.3832404.360063047381979287.git-patchwork-notify@kernel.org/)
that got positive feedback so far.
So if you're ok with having input differ in style from (up to now) all
other subsystems, I can rework the patch.
Best regards
Uwe
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH] HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
From: sashiko-bot @ 2026-05-30 16:16 UTC (permalink / raw)
To: Jinmo Yang; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260530155930.128183-1-jinmo44.yang@gmail.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Critical] A malicious USB device can trigger a massive heap buffer overflow by sending a HID report larger than the wacom pen_fifo capacity (e.g., > PAGE_SIZE).
--
commit 2421694466917c461a3ca5695c18542757de2d44
Author: Jinmo Yang <jinmo44.yang@gmail.com>
HID: wacom: use GFP_ATOMIC in wacom_wac_queue_flush()
wacom_wac_queue_flush() is called via the .raw_event callback
(wacom_raw_event → wacom_wac_pen_serial_enforce → wacom_wac_queue_flush).
For USB HID devices, this callback is invoked from hid_irq_in(), which
is a URB completion handler running in atomic context. Using GFP_KERNEL
in this path can sleep, leading to a "scheduling while atomic" bug.
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..a25400b3e658d 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -74,7 +74,7 @@ static void wacom_wac_queue_flush(struct hid_device *hdev,
> unsigned int count;
> int err;
>
> - buf = kzalloc(size, GFP_KERNEL);
> + buf = kzalloc(size, GFP_ATOMIC);
> if (!buf) {
> kfifo_skip(fifo);
[Severity: Critical]
This is a pre-existing issue, but does this usage of kfifo_skip() point to a
similar problem in wacom_wac_queue_insert() in drivers/hid/wacom_sys.c?
If a malicious device spoofs a Wacom device with WACOM_QUIRK_TOOLSERIAL and
sends a report larger than PAGE_SIZE, wouldn't wacom_wac_queue_insert() be
called with size > PAGE_SIZE?
Since pen_fifo capacity is bounded to at most PAGE_SIZE, kfifo_avail(fifo) can
never naturally be >= size, causing the code to loop and blindly call
kfifo_skip(fifo):
drivers/hid/wacom_sys.c:wacom_wac_queue_insert() {
...
while (kfifo_avail(fifo) < size) {
if (!warned)
hid_warn(hdev, "%s: kfifo has filled, starting to drop events\n", __func__);
warned = true;
kfifo_skip(fifo);
}
kfifo_in(fifo, raw_data, size);
...
Doesn't kfifo_skip(fifo) advance fifo->out even when empty, underflowing
fifo->in - fifo->out to a massive unsigned value?
Could this trick kfifo_avail(fifo) into returning a huge number, breaking the
loop and allowing kfifo_in() to write up to 16KB out of bounds into the 4KB
fifo->data buffer via lib/kfifo.c:kfifo_copy_in()?
lib/kfifo.c:kfifo_copy_in() {
...
memcpy(fifo->data, src + l, len - l);
...
> continue;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260530155930.128183-1-jinmo44.yang@gmail.com?part=1
^ permalink raw reply
* [PATCH 6.1 060/969] HID: alps: fix NULL pointer dereference in alps_raw_event()
From: Greg Kroah-Hartman @ 2026-05-30 15:53 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, Masaki Ota, linux-input, Jiri Kosina
In-Reply-To: <20260530160300.485627683@linuxfoundation.org>
6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 1badfc4319224820d5d890f8eab6aa52e4e83339 upstream.
Commit ecfa6f34492c ("HID: Add HID_CLAIMED_INPUT guards in raw_event
callbacks missing them") attempted to fix up the HID drivers that had
missed the previous fix that was done in 2ff5baa9b527 ("HID: appleir:
Fix potential NULL dereference at raw event handle"), but the alps
driver was missed.
Fix this up by properly checking in the hid-alps driver that it had been
claimed correctly before attempting to process the raw event.
Fixes: 73196ebe134d ("HID: alps: add support for Alps T4 Touchpad device")
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: Masaki Ota <masaki.ota@jp.alps.com>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-alps.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-alps.c
+++ b/drivers/hid/hid-alps.c
@@ -437,6 +437,9 @@ static int alps_raw_event(struct hid_dev
int ret = 0;
struct alps_dev *hdata = hid_get_drvdata(hdev);
+ if (!(hdev->claimed & HID_CLAIMED_INPUT) || !hdata->input)
+ return 0;
+
switch (hdev->product) {
case HID_PRODUCT_ID_T4_BTNLESS:
ret = t4_raw_event(hdata, data, size);
^ permalink raw reply
* [PATCH 6.1 061/969] HID: core: clamp report_size in s32ton() to avoid undefined shift
From: Greg Kroah-Hartman @ 2026-05-30 15:53 UTC (permalink / raw)
To: stable
Cc: Greg Kroah-Hartman, patches, stable, Jiri Kosina,
Benjamin Tissoires, linux-input, Jiri Kosina
In-Reply-To: <20260530160300.485627683@linuxfoundation.org>
6.1-stable review patch. If anyone has any objections, please let me know.
------------------
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
commit 69c02ffde6ed4d535fa4e693a9e572729cad3d0d upstream.
s32ton() shifts by n-1 where n is the field's report_size, a value that
comes directly from a HID device. The HID parser bounds report_size
only to <= 256, so a broken HID device can supply a report descriptor
with a wide field that triggers shift exponents up to 256 on a 32-bit
type when an output report is built via hid_output_field() or
hid_set_field().
Commit ec61b41918587 ("HID: core: fix shift-out-of-bounds in
hid_report_raw_event") added the same n > 32 clamp to the function
snto32(), but s32ton() was never given the same fix as I guess syzbot
hadn't figured out how to fuzz a device the same way.
Fix this up by just clamping the max value of n, just like snto32()
does.
Cc: stable <stable@kernel.org>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <bentiss@kernel.org>
Cc: linux-input@vger.kernel.org
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Jiri Kosina <jkosina@suse.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
drivers/hid/hid-core.c | 3 +++
1 file changed, 3 insertions(+)
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1358,6 +1358,9 @@ static u32 s32ton(__s32 value, unsigned
if (!value || !n)
return 0;
+ if (n > 32)
+ n = 32;
+
a = value >> (n - 1);
if (a && a != -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