* Re: [PATCH v4 09/11] dt-bindings: input: Document hid-over-spi DT schema
From: sashiko-bot @ 2026-06-09 4:50 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: linux-input, robh, conor+dt, dmitry.torokhov, devicetree
In-Reply-To: <20260609-send-upstream-v4-9-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `read-opcode` and `write-opcode` properties are defined as `uint8`, which will cause the driver to fail probing with `-EOVERFLOW` if using u32 reads.
- [Low] Missing 'reg' property in the 'required' list for an SPI peripheral.
--
commit 455b6048d6e6dd86252693c3c8edc0762c5dbba4
Author: Jingyuan Liang <jingyliang@chromium.org>
dt-bindings: input: Document hid-over-spi DT schema
Documentation describes the required and optional properties for
implementing Device Tree for a Microsoft G6 Touch Digitizer that
supports HID over SPI Protocol 1.0 specification.
> diff --git a/Documentation/devicetree/bindings/input/hid-over-spi.yaml b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
> new file mode 100644
> index 0000000000000..27cf311e0aab0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/hid-over-spi.yaml
[ ... ]
> + read-opcode:
> + $ref: /schemas/types.yaml#/definitions/uint8
> + description:
> + Value to be used in Read Approval packets. 1 byte.
> +
> + write-opcode:
> + $ref: /schemas/types.yaml#/definitions/uint8
> + description:
> + Value to be used in Write Approval packets. 1 byte.
[Severity: High]
Will defining these as uint8 cause the driver probe to fail?
In the driver (drivers/hid/spi-hid/spi-hid-of.c), spi_hid_of_populate_config()
reads these using device_property_read_u32().
If a conforming device tree provides 1-byte properties,
device_property_read_u32() will return -EOVERFLOW because the property length
is less than 4 bytes, causing the probe to abort.
Should the driver be updated to use device_property_read_u8(), or should
these schema properties be uint32?
> +
> +required:
> + - compatible
> + - interrupts
> + - reset-gpios
> + - vdd-supply
[Severity: Low]
Should 'reg' be included in this required list?
Although spi-controller.yaml enforces 'reg' for child nodes matching the
@[0-9a-f]+$ pattern, a malformed node without a unit address would bypass
this check and silently pass schema validation against this binding.
> + - input-report-header-address
> + - input-report-body-address
> + - output-report-address
> + - read-opcode
> + - write-opcode
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=9
^ permalink raw reply
* Re: [PATCH v4 08/11] HID: spi_hid: add device tree support for SPI over HID
From: sashiko-bot @ 2026-06-09 4:54 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: linux-input, dmitry.torokhov, robh, conor+dt, devicetree
In-Reply-To: <20260609-send-upstream-v4-8-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap buffer overflow via attacker-controlled `report_length`.
- [Medium] API misuse: Calling `gpiod_set_value` instead of `gpiod_set_value_cansleep`.
- [Medium] Hardware backpowering risk due to improper GPIO initialization.
- [Low] Log spam during boot due to unhandled -EPROBE_DEFER.
--
commit 0c5bcceb42b14d25affd5a3652cde4d4a22e95c2
Author: Jarrett Schultz <jaschultz@microsoft.com>
HID: spi_hid: add device tree support for SPI over HID
This commit adds Open Firmware (Device Tree) transport support for HID
over SPI devices. It introduces the spi_hid_of module to configure and
manage power and reset lines for these devices based on device tree
properties.
> diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
> new file mode 100644
> index 0000000000000..ba7d5338f5d89
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-of.c
> @@ -0,0 +1,246 @@
[ ... ]
> +static int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> + struct device *dev)
> +{
> + int error;
> + u32 val;
[ ... ]
> + conf->supply = devm_regulator_get(dev, "vdd");
> + if (IS_ERR(conf->supply)) {
> + if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
> + dev_err(dev, "Failed to get regulator: %ld.",
> + PTR_ERR(conf->supply));
> + return PTR_ERR(conf->supply);
> + }
> + conf->supply_enabled = false;
> +
> + conf->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);
[Severity: Medium]
Could initializing the reset GPIO with GPIOD_OUT_LOW cause hardware
backpowering?
If the GPIO is specified as active-low in the Device Tree, a logical 0
drives the physical pin high. Because this initialization happens during
probe before the device regulator is turned on in spi_hid_of_power_up(),
a high voltage might be driven into an unpowered peripheral. This could
forward-bias ESD diodes and backpower the device.
> + if (IS_ERR(conf->reset_gpio)) {
> + dev_err(dev, "%s: error getting reset GPIO.", __func__);
[Severity: Low]
Does this code unnecessarily log errors during normal boot sequencing?
If devm_gpiod_get() returns -EPROBE_DEFER, the error is unconditionally
logged by dev_err() before being returned.
> + return PTR_ERR(conf->reset_gpio);
> + }
> +
> + return 0;
> +}
[ ... ]
> +static int spi_hid_of_assert_reset(struct spihid_ops *ops)
> +{
> + struct spi_hid_of_config *conf = container_of(ops,
> + struct spi_hid_of_config,
> + ops);
> +
> + gpiod_set_value(conf->reset_gpio, 1);
[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead of gpiod_set_value()?
The assert_reset callback is invoked from sleepable contexts like probe
and workqueues. If the reset GPIO is attached to a sleepable controller
such as an I2C or SPI expander, calling the non-sleeping gpiod_set_value()
will trigger a WARN_ON in the GPIO core.
> + return 0;
> +}
> +
> +static int spi_hid_of_deassert_reset(struct spihid_ops *ops)
> +{
> + struct spi_hid_of_config *conf = container_of(ops,
> + struct spi_hid_of_config,
> + ops);
> +
> + gpiod_set_value(conf->reset_gpio, 0);
[Severity: Medium]
Similarly, should this use gpiod_set_value_cansleep() since deassert_reset
can also be called from sleepable contexts?
> + return 0;
> +}
[ ... ]
> +static int spi_hid_of_probe(struct spi_device *spi)
> +{
> + struct device *dev = &spi->dev;
> + struct spi_hid_of_config *config;
> + int error;
[ ... ]
> + error = spi_hid_of_populate_config(config, dev);
> + if (error) {
> + dev_err(dev, "%s: unable to populate config data.", __func__);
[Severity: Low]
Does this code unnecessarily log a second error on probe deferral?
If spi_hid_of_populate_config() returns -EPROBE_DEFER, dev_err()
will unconditionally log it here as well.
> + return error;
> + }
> +
> + return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
> +}
[ ... ]
In relation to the core code enabled by this transport driver:
[Severity: High]
Does the core input handling allow a heap buffer overflow via an
attacker-controlled report_length?
In drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_irq(), the input report
header is read, and header.report_length is checked:
if (shid->desc.max_input_length != 0 &&
header->report_length > shid->desc.max_input_length) {
The max_input_length is completely controlled by the SPI device and can
be set to 0 to bypass the check entirely, or set to a large value.
The driver then initiates a DMA read:
error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
false);
Because the actual allocated buffer size (shid->bufsize) is determined
independently by the HID report descriptors, a malicious device could
send small report descriptors but huge report_length values, causing a
DMA read directly past the bounds of the shid->input->body heap buffer.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=8
^ permalink raw reply
* Re: [PATCH v4 04/11] HID: spi-hid: add spi-hid driver HID layer
From: sashiko-bot @ 2026-06-09 4:54 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: devicetree, dmitry.torokhov, linux-input, conor+dt, robh
In-Reply-To: <20260609-send-upstream-v4-4-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing bounds check against allocated buffer size leads to a kernel heap buffer overflow.
- [High] Out-of-bounds write by 1 byte when copying the `HID_REQ_GET_REPORT` response in `spi_hid_ll_raw_request`.
- [High] Unsynchronized access to the shared `shid->response` buffer in `spi_hid_ll_raw_request`.
- [High] Timeout or interruption in `spi_hid_sync_request` leaves the device state and completion structure out-of-sync.
- [High] Device probe fails for standard HID devices with small reports because `spi_hid_ll_start` returns `-EINVAL`.
--
commit 475a21113f187310ac25983762798463f8e5115e
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add spi-hid driver HID layer
Add HID low level driver callbacks to register SPI as a HID driver, and
an external touch device as a HID device.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..72c2e1ce3e8d9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -84,11 +185,455 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
> +static int spi_hid_sync_request(struct spi_hid *shid,
> + struct spi_hid_output_report *report)
> +{
> + struct device *dev = &shid->spi->dev;
> + int error;
> +
> + guard(mutex)(&shid->output_lock);
> +
> + reinit_completion(&shid->output_done);
> +
> + error = __spi_hid_send_output_report(shid, report);
> + if (error)
> + return error;
> +
> + error = wait_for_completion_interruptible_timeout(&shid->output_done,
> + msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
> + if (error == 0) {
> + dev_err(dev, "Response timed out\n");
> + return -ETIMEDOUT;
> + }
[Severity: High]
If this times out or is interrupted, the function returns and releases
shid->output_lock, but does the hardware stop processing the request?
If the hardware is still running and sends a late response, could it
prematurely complete shid->output_done for a subsequent request, permanently
desynchronizing the driver and the device?
[ ... ]
> +static int spi_hid_alloc_buffers(struct spi_hid *shid, size_t report_size)
> +{
> + struct device *dev = &shid->spi->dev;
> + int inbufsize = round_up(sizeof(shid->input->header) +
> + sizeof(shid->input->body) + report_size, 4);
> + int outbufsize = round_up(sizeof(shid->output->header) + report_size, 4);
> + void *tmp;
> +
> + tmp = devm_krealloc(dev, shid->output, outbufsize, GFP_KERNEL | __GFP_ZERO);
> + if (!tmp)
> + return -ENOMEM;
> + shid->output = tmp;
> +
> + tmp = devm_krealloc(dev, shid->input, inbufsize, GFP_KERNEL | __GFP_ZERO);
> + if (!tmp)
> + return -ENOMEM;
> + shid->input = tmp;
> +
> + tmp = devm_krealloc(dev, shid->response, inbufsize, GFP_KERNEL | __GFP_ZERO);
[Severity: Critical]
When allocating these buffers based on report_size, is there a corresponding
bounds check against the allocated size when reading from the SPI device?
In spi_hid_dev_irq() and spi_hid_input_sync(), the driver appears to read
header.report_length bytes into shid->input->body. Furthermore, in
spi_hid_response_handler(), memcpy copies body.content_len bytes into
shid->response->content.
Since these lengths are provided by the device, could a malicious or
malfunctioning device send a larger length and overflow the allocated heap
buffers?
[ ... ]
> +static int spi_hid_ll_start(struct hid_device *hid)
> +{
> + struct spi_device *spi = hid->driver_data;
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + int error = 0;
> + u16 bufsize = 0;
> +
> + spi_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
> + spi_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
> + spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
> +
> + if (bufsize < HID_MIN_BUFFER_SIZE) {
> + dev_err(&spi->dev,
> + "HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
> + bufsize);
> + return -EINVAL;
> + }
[Severity: High]
Does this check unconditionally reject standard HID devices (like mice or
keyboards) that legitimately have maximum report sizes smaller than
HID_MIN_BUFFER_SIZE (64 bytes)?
Should the driver enforce a minimum allocation size (for example, by padding
bufsize) instead of refusing to probe the device?
[ ... ]
> +static int spi_hid_ll_raw_request(struct hid_device *hid,
> + unsigned char reportnum, __u8 *buf,
> + size_t len, unsigned char rtype, int reqtype)
> +{
> + struct spi_device *spi = hid->driver_data;
> + struct spi_hid *shid = spi_get_drvdata(spi);
> + struct device *dev = &spi->dev;
> + int ret;
> +
> + switch (reqtype) {
[ ... ]
> + case HID_REQ_GET_REPORT:
> + ret = spi_hid_get_request(shid, reportnum);
[Severity: High]
After spi_hid_get_request() returns, shid->output_lock is no longer held.
Could another thread concurrently call spi_hid_ll_raw_request() or
spi_hid_ll_output_report(), acquire the lock, and overwrite shid->response
before the current thread copies it?
> + if (ret) {
> + dev_err(dev, "failed to get report\n");
> + return ret;
> + }
> +
> + ret = min_t(size_t, len,
> + (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
> + buf[0] = shid->response->body[3];
> + memcpy(&buf[1], &shid->response->content, ret);
[Severity: High]
Does this memcpy() cause an out-of-bounds write of 1 byte?
The code copies 'ret' bytes starting at '&buf[1]', which means it writes a
total of 'ret + 1' bytes into 'buf'. If 'ret' equals the allocated buffer
size 'len', it will write 'len + 1' bytes. Should the memcpy length
parameter be 'ret - 1'?
> + break;
> + default:
> + dev_err(dev, "invalid request type\n");
> + return -EIO;
> + }
> +
> + return ret;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=4
^ permalink raw reply
* Re: [PATCH v4 06/11] HID: spi_hid: add spi_hid traces
From: sashiko-bot @ 2026-06-09 4:55 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: robh, dmitry.torokhov, linux-input, devicetree, conor+dt
In-Reply-To: <20260609-send-upstream-v4-6-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
New issues:
- [High] The `____cacheline_aligned` attribute was dropped from the DMA-safe transfer buffers `read_approval_header` and `read_approval_body` when the `spi_hid` struct was moved to a header file. This can lead to DMA cache coherency issues and memory corruption on architectures with non-coherent caches.
- [Medium] The `trace_spi_hid_input_sync` tracepoint logs the contents of the receive buffer (`rx_buf`) *before* the SPI transfer completes, leaking uninitialized heap memory or stale data from previous transfers.
Pre-existing issues:
- [Critical] A missing bounds check on `header.report_length` against the dynamically allocated buffer size allows a massive out-of-bounds DMA write, leading to a heap buffer overflow.
- [High] Concurrent reallocation of the shared `shid->input` buffer in `spi_hid_ll_parse` while the interrupt handler accesses it leads to a Use-After-Free.
- [High] Handling `HID_REQ_GET_REPORT` in `spi_hid_ll_raw_request` causes an out-of-bounds write to the caller's buffer by exactly 1 byte.
- [High] Concurrent overwriting of `shid->response` by the IRQ handler while a waiting thread is actively processing it leads to data corruption.
--
commit 2915829fcdae73aed3b7b4c76739fccf90310d7d
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi_hid: add spi_hid traces
Add traces for purposed of debugging spi_hid driver.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index f6ea2d4365a70..ef527999d6dc6 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -295,6 +225,11 @@ static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
> spi_message_init_with_transfers(&shid->input_message,
> shid->input_transfer, 2);
>
> + trace_spi_hid_input_sync(shid, shid->input_transfer[0].tx_buf,
> + shid->input_transfer[0].len,
> + shid->input_transfer[1].rx_buf,
> + shid->input_transfer[1].len, 0);
> +
> error = spi_sync(shid->spi, &shid->input_message);
[Severity: Medium]
Does this tracepoint log the contents of the receive buffer before the SPI
transfer actually completes? Since spi_sync() hasn't executed yet, it appears
this will blindly copy uninitialized heap memory or stale data from previous
transfers into the trace output.
[ ... ]
> @@ -528,6 +468,8 @@ static int spi_hid_input_report_handler(struct spi_hid *shid,
> static void spi_hid_response_handler(struct spi_hid *shid,
> struct input_report_body_header *body)
> {
> + trace_spi_hid_response_handler(shid);
> +
> shid->response_length = body->content_len;
> /* completion_done returns 0 if there are waiters, otherwise 1 */
> if (completion_done(&shid->output_done)) {
[Severity: High]
This is a pre-existing issue, but is the completion_done() check sufficient
to protect against data corruption?
When a waiting thread wakes up from wait_for_completion_interruptible_timeout()
in spi_hid_sync_request(), the completion's internal done count is decremented
to 0. A newly arriving interrupt will see completion_done() == 0, and will
overwrite shid->response while the awakened thread is concurrently reading it
outside of the output_lock.
[ ... ]
> @@ -867,6 +815,9 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> struct spi_hid_input_header header;
> int error = 0;
>
> + trace_spi_hid_dev_irq(shid, irq);
> + trace_spi_hid_header_transfer(shid);
> +
> scoped_guard(mutex, &shid->io_lock) {
[Severity: High]
This is a pre-existing issue, but does concurrent buffer reallocation in
spi_hid_ll_parse() lead to a use-after-free here?
spi_hid_ll_parse() calls spi_hid_alloc_buffers() (which uses devm_krealloc())
without disabling the IRQ or holding the io_lock. If devm_krealloc() frees
the old buffer while the IRQ handler is concurrently configuring DMA transfers
into shid->input, will the IRQ handler write into freed memory?
[ ... ]
> @@ -916,6 +874,12 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> goto out;
> }
>
> + trace_spi_hid_input_body_complete(shid, shid->input_transfer[0].tx_buf,
> + shid->input_transfer[0].len,
> + shid->input_transfer[1].rx_buf,
> + shid->input_transfer[1].len,
> + shid->input_message.status);
> +
> if (shid->input_message.status < 0) {
[Severity: Critical]
This is a pre-existing issue, but is there a missing bounds check that could
lead to an out-of-bounds DMA write?
In spi_hid_bus_validate_header(), the validation only checks if
header.report_length > shid->desc.max_input_length. If a malicious device
advertises a max_input_length of 0, the check is bypassed entirely. The driver
then unconditionally performs a DMA read of header.report_length bytes into
shid->input->body, which could overflow the dynamically allocated bufsize.
[Severity: High]
This is a pre-existing issue elsewhere in this file, but does handling
HID_REQ_GET_REPORT in spi_hid_ll_raw_request() cause an out-of-bounds write?
In spi_hid_ll_raw_request():
ret = min_t(size_t, len,
(shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
buf[0] = shid->response->body[3];
memcpy(&buf[1], &shid->response->content, ret);
The driver writes the 1-byte ID to buf[0] and then copies ret bytes into
&buf[1], meaning it writes a total of ret + 1 bytes. If ret equals the
caller's buffer size (len), doesn't this overflow the destination buffer by
exactly 1 byte?
> diff --git a/drivers/hid/spi-hid/spi-hid-core.h b/drivers/hid/spi-hid/spi-hid-core.h
> new file mode 100644
> index 0000000000000..293e2cfcfbf74
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.h
[ ... ]
> @@ -0,0 +1,91 @@
> +/* Driver context */
> +struct spi_hid {
[ ... ]
> + struct completion output_done;
> +
> + u8 read_approval_header[SPI_HID_READ_APPROVAL_LEN];
> + u8 read_approval_body[SPI_HID_READ_APPROVAL_LEN];
> +
> + u32 report_descriptor_crc32; /* HID report descriptor crc32 checksum. */
[Severity: High]
When moving struct spi_hid to this new header, the ____cacheline_aligned
attribute was dropped from the read_approval_header and read_approval_body
buffers.
Could this lead to DMA cache coherency issues and memory corruption on
architectures with non-coherent caches, since these arrays might now share
cachelines with adjacent fields?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=6
^ permalink raw reply
* Re: [PATCH v4 03/11] HID: spi-hid: add transport driver skeleton for HID over SPI bus
From: sashiko-bot @ 2026-06-09 4:56 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: robh, conor+dt, devicetree, dmitry.torokhov, linux-input
In-Reply-To: <20260609-send-upstream-v4-3-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Embedded arrays `read_approval_header` and `read_approval_body` are used for SPI DMA transfers without cacheline alignment, risking memory corruption.
- [Medium] Unchecked return values of reset operations in `spi_hid_dev_init` and `spi_hid_resume`
--
commit ea35a3a23739acaca367a8d250ef25c96d22df36
Author: Angela Czubak <acz@semihalf.com>
HID: spi-hid: add transport driver skeleton for HID over SPI bus
Create spi-hid folder and add Kconfig and Makefile for spi-hid driver.
Add basic device structure, definitions, and probe/remove functions.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> new file mode 100644
> index 0000000000000..02a7608c4b882
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -0,0 +1,213 @@
[ ... ]
> +/* Driver context */
> +struct spi_hid {
> + struct spi_device *spi; /* spi device. */
> + struct hid_device *hid; /* pointer to corresponding HID dev. */
> +
> + struct spihid_ops *ops;
> + struct spi_hid_conf *conf;
> +
> + enum hidspi_power_state power_state;
> +
> + u32 regulator_error_count;
> + int regulator_last_error;
> + u32 bus_error_count;
> + int bus_last_error;
> + u32 dir_count; /* device initiated reset count. */
> +};
[Severity: High]
Does embedding the read_approval_header and read_approval_body arrays directly
in this struct for SPI DMA transfers risk memory corruption?
When these buffers are passed as tx_buf to spi_sync() (e.g. in
spi_hid_input_sync()), they lack the ____cacheline_aligned directive. On
architectures with non-coherent DMA, cache maintenance operations could
encompass adjacent struct members.
Concurrent CPU writes to those adjacent members during the DMA mapping window
will be destroyed, leading to memory corruption.
Should these buffers be cacheline aligned, or dynamically allocated?
[ ... ]
> +int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
> + struct spi_hid_conf *conf)
> +{
[ ... ]
> + /*
> + * At the end of probe we initialize the device:
> + * 0) assert reset, bias the interrupt line
> + * 1) sleep minimal reset delay
> + * 2) request IRQ
> + * 3) power up the device
> + * 4) deassert reset (high)
> + * After this we expect an IRQ with a reset response.
> + */
> +
> + shid->ops->assert_reset(shid->ops);
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
[ ... ]
> + error = shid->ops->power_up(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power up\n", __func__);
> + return error;
> + }
> +
> + shid->ops->deassert_reset(shid->ops);
[Severity: Medium]
Can hardware reset failures be masked here?
The assert_reset and deassert_reset callbacks return integer error codes.
However, their return values are ignored in spi_hid_core_probe() (as well as
resume paths).
If deassert_reset() fails, the device may remain in reset, causing the driver
to hang waiting for a reset response IRQ that will never arrive.
Should these return values be checked and handled?
> +
> + dev_dbg(dev, "%s: d3 -> %s\n", __func__,
> + spi_hid_power_mode_string(shid->power_state));
> +
> + return 0;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=3
^ permalink raw reply
* Re: [PATCH v4 11/11] HID: spi-hid: add panel follower support
From: sashiko-bot @ 2026-06-09 4:58 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: dmitry.torokhov, robh, devicetree, linux-input, conor+dt
In-Reply-To: <20260609-send-upstream-v4-11-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues:
- [High] Leak of wake IRQ structure on probe failure or driver unbind for panel followers.
- [High] Unbalanced regulator power_down during remove() for unbound panel followers.
- [High] Missing power_state update in spi_hid_dev_init() breaks device and leaks regulator after suspend.
Pre-existing issues:
- [High] Preexisting Use-After-Free due to race between cancel_work_sync() and hid_destroy_device() timeouts.
--
commit 6b1c4605757961c511d63ab9f2108f26f48fd916
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add panel follower support
Add support to spi-hid to be a panel follower.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 517f069134775..27f25d95ed280 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
> @@ -1281,6 +1281,106 @@ const struct attribute_group *spi_hid_groups[] = {
[ ... ]
> +static int spi_hid_dev_init(struct spi_hid *shid)
> +{
> + struct spi_device *spi = shid->spi;
> + struct device *dev = &spi->dev;
> + int error;
> +
> + shid->ops->assert_reset(shid->ops);
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> + error = shid->ops->power_up(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power up\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + return error;
> + }
> +
> + shid->ops->deassert_reset(shid->ops);
> +
> + enable_irq(spi->irq);
> +
> + return 0;
> +}
[Severity: High]
If a panel follower suspends before it successfully receives its hid_version,
spi_hid_suspend() will legitimately power down the device and set power_state =
HIDSPI_OFF.
Upon resume, spi_hid_panel_follower_work() retries initialization by calling
spi_hid_dev_init(). Since spi_hid_dev_init() does not update power_state to
HIDSPI_ON, does the state remain HIDSPI_OFF?
If so, when the reset response interrupt arrives, would spi_hid_dev_irq() drop
it because it believes the device is off, leaving the device broken?
Furthermore, would any subsequent suspend return early because power_state ==
HIDSPI_OFF, failing to power down the regulator and causing a power leak?
[ ... ]
> +static int spi_hid_register_panel_follower(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> +
> + shid->panel_follower.funcs = &spi_hid_panel_follower_prepare_funcs;
> +
> + /*
> + * If we're not in control of our own power up/power down then we can't
> + * do the logic to manage wakeups. Give a warning if a user thought
> + * that was possible then force the capability off.
> + */
> + if (device_can_wakeup(dev)) {
> + dev_warn(dev, "Can't wakeup if following panel\n");
> + device_set_wakeup_capable(dev, false);
> + }
[Severity: High]
Does explicitly executing device_set_wakeup_capable(dev, false) mutate the
dev->power.can_wakeup flag to false, which would then break the symmetry of the
wake IRQ cleanup conditions?
In spi_hid_core_probe(), the wake IRQ is successfully registered if
device_may_wakeup(dev) is true. During error cleanup (err_wake_irq) or driver
removal (spi_hid_core_remove()), the code conditionally calls
dev_pm_clear_wake_irq(dev) based on if (device_may_wakeup(dev)).
Since the capability was previously cleared here, will this check evaluate to
false and cause the driver to skip the cleanup, leaking the wake IRQ
registration?
[ ... ]
> @@ -1300,6 +1400,7 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
> shid->ops = ops;
> shid->conf = conf;
> set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> + shid->is_panel_follower = drm_is_panel_follower(&spi->dev);
[Severity: High]
During spi_hid_core_probe(), shid->power_state is unconditionally initialized
to HIDSPI_ON. For a panel follower, the actual hardware initialization and
regulator power_up() are deferred to spi_hid_panel_follower_work().
If the driver is unbound (or probe fails gracefully after device registration)
before the panel prepares, power_up() is never called. However,
spi_hid_core_remove() checks if (shid->power_state != HIDSPI_OFF) and calls
power_down().
Will this trigger a power_down() on an already disabled regulator, resulting in
an unbalanced regulator use-count?
[ ... ]
> @@ -1376,15 +1470,21 @@ void spi_hid_core_remove(struct spi_device *spi)
> struct device *dev = &spi->dev;
> int error;
>
> - disable_irq(spi->irq);
> + if (shid->is_panel_follower)
> + drm_panel_remove_follower(&shid->panel_follower);
> + else
> + disable_irq(spi->irq);
> +
> cancel_work_sync(&shid->reset_work);
>
> spi_hid_stop_hid(shid);
[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk due to a race
between cancel_work_sync() and hid_destroy_device() timeouts?
In spi_hid_core_remove(), cancel_work_sync(&shid->reset_work) is called prior
to spi_hid_stop_hid(shid), which destroys the HID device. Because
disable_irq(spi->irq) is called beforehand, any concurrent HID request (like a
userspace GET_REPORT ioctl) waiting in
wait_for_completion_interruptible_timeout() will inevitably time out.
Upon timing out, the sync request handler sets the error flag and executes
schedule_work(&shid->reset_work). Since this timeout can occur after
cancel_work_sync() has already finished but before remove() concludes, the
work is freshly queued. The remove() function then frees shid, resulting in
the reset_work executing on freed memory.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=11
^ permalink raw reply
* Re: [PATCH v4 05/11] HID: spi-hid: add HID SPI protocol implementation
From: sashiko-bot @ 2026-06-09 5:00 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: conor+dt, linux-input, robh, devicetree, dmitry.torokhov
In-Reply-To: <20260609-send-upstream-v4-5-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
New issues:
- [Critical] Missing validation of `report_length` against the allocated DMA buffer size allows an out-of-bounds write of up to ~64KB during SPI input read.
- [Critical] Use-After-Free of `shid->input` DMA buffers via concurrent reallocation and IRQ handling.
- [Critical] Out-of-bounds write in `spi_hid_response_handler` using device-supplied length.
- [High] Incorrect sizing of arrays in `struct spi_hid_input_buf` drops the Report ID and causes an out-of-bounds read when reporting to the HID core.
- [High] Returning `IRQ_HANDLED` when the device is off without servicing the interrupt causes an interrupt storm.
- [High] Missing power state update in `spi_hid_error_handler` abort path leads to an infinite interrupt storm.
- [High] Use-After-Free (UAF) on driver unbind due to missing synchronization with active HID raw requests.
- [High] Holding `io_lock` over `hid_input_report` creates an ABBA lock inversion and potential self-deadlock with `output_lock`.
- [High] Missing validation of response type in `spi_hid_response_handler` leaks stale data to userspace.
Pre-existing issues:
- [High] Handling of `HID_REQ_GET_REPORT` in `spi_hid_ll_raw_request` copies too many bytes into the output buffer, overflowing the caller's allocation by 1 byte and reading out of bounds.
- [Medium] Concurrent calls to `HID_REQ_GET_REPORT` cause a data race on the shared `shid->response` buffer, corrupting data returned to userspace.
--
commit 49a8cad2bdbcc64aa9d451335312691879579a95
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add HID SPI protocol implementation
This driver follows HID Over SPI Protocol Specification 1.0 available at
https://www.microsoft.com/en-us/download/details.aspx?id=103325. The
initial version of the driver does not support: 1) multi-fragment input
reports, 2) sending GET_INPUT and COMMAND output report types and
processing their respective acknowledge input reports, and 3) device
sleep power state.
diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
index 72c2e1ce3e8d9..f6ea2d4365a70 100644
--- a/drivers/hid/spi-hid/spi-hid-core.c
+++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
@@ -187,15 +336,64 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> + struct device *dev = &shid->spi->dev;
> + int error;
> +
> + guard(mutex)(&shid->power_lock);
> + if (shid->power_state == HIDSPI_OFF)
> + return;
> +
> + guard(disable_irq)(&shid->spi->irq);
> +
> + if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> + dev_err(dev, "unresponsive device, aborting\n");
> + spi_hid_stop_hid(shid);
> + shid->ops->assert_reset(shid->ops);
> + error = shid->ops->power_down(shid->ops);
> + if (error) {
> + dev_err(dev, "failed to disable regulator\n");
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + }
> + return;
> + }
[Severity: High]
Does this error path need to update the power state?
When reset_attempts exceeds the maximum, this function powers down the
device and returns immediately without setting shid->power_state to
HIDSPI_OFF.
Because this function uses guard(disable_irq), returning automatically
re-enables the interrupt. Since the device is unpowered, it will assert
its IRQ line and trigger the handler again. Since power_state is still
HIDSPI_ON, can this cause an infinite interrupt storm?
[ ... ]
@@ -263,6 +462,88 @@ static int spi_hid_sync_request(struct spi_hid *shid,
> +static int spi_hid_input_report_handler(struct spi_hid *shid,
> + struct spi_hid_input_buf *buf)
> +{
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_input_report r;
> + int error = 0;
> +
> + guard(mutex)(&shid->io_lock);
[Severity: High]
Can this cause an ABBA lock inversion with output_lock?
This function holds io_lock before calling into the generic HID subsystem.
If a BPF program attached to the HID core synchronously triggers
hid_hw_raw_request during this call, it routes back to the driver
via spi_hid_ll_raw_request(), which calls spi_hid_sync_request()
(acquiring output_lock) and then __spi_hid_send_output_report(), which
attempts to acquire io_lock. Could this self-deadlock the thread?
> +
> + if (!test_bit(SPI_HID_READY, &shid->flags) ||
> + test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
> + dev_err(dev, "HID not ready\n");
> + return 0;
> + }
> +
> + spi_hid_input_report_prepare(buf, &r);
> +
> + error = hid_input_report(shid->hid, HID_INPUT_REPORT,
> + r.content - 1, r.content_length + 1, 1);
[Severity: High]
Does passing r.content - 1 drop the Report ID?
Looking at struct spi_hid_input_buf, the body array is sized to
SPI_HID_READ_APPROVAL_LEN (which is 5 bytes). However, the actual protocol
body header is only 4 bytes. This means the first byte of the content lands
in buf->body[4], while the Report ID is in buf->body[3].
Since this passes r.content - 1 (which points to buf->body[4]) to
hid_input_report, it completely skips the Report ID. Because it also passes
r.content_length + 1 as the size, does this read one byte past the end of
the received SPI transfer into uninitialized heap memory?
[ ... ]
> +static void spi_hid_response_handler(struct spi_hid *shid,
> + struct input_report_body_header *body)
> +{
> + shid->response_length = body->content_len;
> + /* completion_done returns 0 if there are waiters, otherwise 1 */
> + if (completion_done(&shid->output_done)) {
> + dev_err(&shid->spi->dev, "Unexpected response report\n");
> + } else {
> + if (body->input_report_type == REPORT_DESCRIPTOR_RESPONSE ||
> + body->input_report_type == GET_FEATURE_RESPONSE) {
> + memcpy(shid->response->body, shid->input->body,
> + sizeof(shid->input->body));
> + memcpy(shid->response->content, shid->input->content,
> + body->content_len);
[Severity: Critical]
Can this memcpy overflow the response buffer?
The memcpy uses body->content_len to copy data into
shid->response->content. While body->content_len is validated against
header.report_length elsewhere, that header length is only bounded by
shid->desc.max_input_length (up to 65535).
Since shid->response is only allocated to shid->bufsize (typically 2048),
can a device sending a large content length cause a heap buffer overflow?
> + }
> + complete(&shid->output_done);
[Severity: High]
Is it safe to unconditionally trigger completion here?
If an unexpected response (such as OUTPUT_REPORT_RESPONSE) arrives,
this completes shid->output_done without updating shid->response->content.
The waiting thread will then wake up and return success. Could this leak
stale kernel heap memory to userspace?
> + }
> +}
[ ... ]
> +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> + struct spi_hid_input_header *header)
> +{
> + struct device *dev = &shid->spi->dev;
> +
> + if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> + dev_err(dev, "Unknown input report version (v 0x%x)\n",
> + header->version);
> + return -EINVAL;
> + }
> +
> + if (shid->desc.max_input_length != 0 &&
> + header->report_length > shid->desc.max_input_length) {
> + dev_err(dev, "Input report body size %u > max expected of %u\n",
> + header->report_length, shid->desc.max_input_length);
> + return -EMSGSIZE;
> + }
[Severity: Critical]
Does this header validation also need to check the allocated buffer size?
This validates header->report_length against shid->desc.max_input_length,
which can be up to 65535, but never validates it against the actually
allocated buffer size (shid->bufsize).
Later in spi_hid_dev_irq(), spi_hid_input_sync() will use this unvalidated
length for a DMA transfer directly into shid->input->body. Could a
malfunctioning device cause a massive kernel heap buffer overflow?
[ ... ]
@@ -371,9 +860,83 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
> +static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> +{
> + struct spi_hid *shid = _shid;
> + struct device *dev = &shid->spi->dev;
> + struct spi_hid_input_header header;
> + int error = 0;
> +
> + scoped_guard(mutex, &shid->io_lock) {
> + error = spi_hid_input_sync(shid, shid->input->header,
> + sizeof(shid->input->header), true);
> + if (error) {
> + dev_err(dev, "Failed to transfer header: %d\n", error);
> + goto err;
> + }
> +
> + if (shid->power_state == HIDSPI_OFF) {
> + dev_warn(dev, "Device is off after header was received\n");
> + goto out;
> + }
[Severity: High]
Can returning IRQ_HANDLED here cause an interrupt storm?
If the device is unpowered, it may float its IRQ line, which is often
level-triggered. Returning IRQ_HANDLED without communicating with the
device to clear the interrupt prevents the spurious IRQ detector from
disabling it. Will this lead to an infinite re-triggering of the interrupt
and a hard lockup?
[ ... ]
> + if (shid->input_message.status < 0) {
> + dev_warn(dev, "Error reading body: %d\n",
> + shid->input_message.status);
> + shid->bus_error_count++;
> + shid->bus_last_error = shid->input_message.status;
> + goto err;
> + }
> + }
> +
> + error = spi_hid_process_input_report(shid, shid->input);
[Severity: Critical]
Is it safe to dereference shid->input outside of the io_lock?
This passes shid->input to spi_hid_process_input_report() entirely outside
of the io_lock scoped guard.
If a concurrent call to spi_hid_alloc_buffers() happens:
tmp = devm_krealloc(dev, shid->input, inbufsize,
GFP_KERNEL | __GFP_ZERO);
This reallocates and frees the buffer while the device IRQ is fully enabled.
Could this lead to a use-after-free if an interrupt fires during or
immediately after the reallocation?
> + if (error) {
> + dev_err(dev, "Failed to process input report: %d\n", error);
> + goto err;
> + }
> +
> +out:
> + return IRQ_HANDLED;
[ ... ]
@@ -762,6 +1335,9 @@ void spi_hid_core_remove(struct spi_device *spi)
> struct device *dev = &spi->dev;
> int error;
>
> + disable_irq(spi->irq);
> + cancel_work_sync(&shid->reset_work);
> +
> spi_hid_stop_hid(shid);
>
> shid->ops->assert_reset(shid->ops);
[Severity: High]
Does driver unbind safely synchronize with active HID raw requests?
This remove function disables the IRQ and destroys the HID device, returning
to allow devres to free the shid struct. However, hid_destroy_device() does
not serialize against active userspace ioctls in hidraw.
If a thread is blocked inside spi_hid_sync_request() holding
shid->output_lock and waiting on shid->output_done:
error = wait_for_completion_interruptible_timeout(&shid->output_done, ...);
Can it wake up after the timeout and access the freed shid memory, causing
a use-after-free?
[Severity: High]
This is a pre-existing issue, but does the handling of HID_REQ_GET_REPORT
overflow the caller's allocation?
Looking at spi_hid_ll_raw_request() from drivers/hid/spi-hid/spi-hid-core.c:
case HID_REQ_GET_REPORT:
ret = spi_hid_get_request(shid, reportnum);
...
ret = min_t(size_t, len,
(shid->response->body[1] |
(shid->response->body[2] << 8)) + 1);
buf[0] = shid->response->body[3];
memcpy(&buf[1], &shid->response->content, ret);
break;
If ret == len, the memcpy writes len bytes starting at &buf[1], consuming
len + 1 bytes in total and overflowing the caller's buf by 1 byte.
It also reads ret bytes from shid->response->content, which only contains
ret - 1 valid payload bytes, causing a 1-byte out-of-bounds read.
[Severity: Medium]
This is a pre-existing issue, but does reading the shared response buffer
require synchronization?
In that same HID_REQ_GET_REPORT path in spi_hid_ll_raw_request(), the code
accesses shid->response->body and shid->response->content without holding
any locks after spi_hid_get_request() drops output_lock.
If multiple threads concurrently issue ioctls, could a second thread
overwrite shid->response via the IRQ handler while the first thread is
executing its memcpy, corrupting data returned to userspace?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=5
^ permalink raw reply
* Re: [PATCH v4 10/11] HID: spi-hid: add power management implementation
From: sashiko-bot @ 2026-06-09 5:00 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: dmitry.torokhov, linux-input, conor+dt, devicetree, robh
In-Reply-To: <20260609-send-upstream-v4-10-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
New issues:
- [High] `spi_hid_resume()` calls `hid_driver_reset_resume()` synchronously before the device signals readiness.
- [High] Error path in `spi_hid_resume()` creates unbalanced IRQ disable depth for wakeup-capable devices.
- [High] Missing synchronization between system suspend and `reset_work` causes a TOCTOU race on `power_state`.
- [High] System suspend cuts device power without synchronizing against ongoing SPI HID requests.
- [High] Unbalanced `enable_irq()` in `spi_hid_resume()` triggers a kernel warning if the device was already `HIDSPI_OFF`.
- [Medium] Conditional cleanup of the wake IRQ leads to a memory leak if userspace disables wakeup.
Pre-existing issues:
- [Critical] Returning `IRQ_HANDLED` for an unpowered device causes an infinite IRQ storm.
--
commit 57585bfd26214992cdd5246d2df2a4622693a915
Author: Jingyuan Liang <jingyliang@chromium.org>
HID: spi-hid: add power management implementation
This commit implements HID over SPI driver power management callbacks, adding
suspend and resume functions for system power state transitions.
> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 698e72102c11e..517f069134775 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -245,6 +247,96 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
> }
> }
>
> +static int spi_hid_suspend(struct spi_hid *shid)
> +{
> + int error;
> + struct device *dev = &shid->spi->dev;
> +
> + guard(mutex)(&shid->power_lock);
[Severity: High]
Does this suspend routine need to synchronize against ongoing SPI I/O?
HID I/O routines like spi_hid_ll_output_report() and spi_hid_ll_raw_request()
acquire output_lock and io_lock to serialize transfers, but they do not check
or lock power_state. Because spi_hid_suspend() acquires power_lock to disable
the device's regulator but does not acquire output_lock or io_lock, nor does
it clear SPI_HID_READY, could it cut hardware power concurrently while a kernel
thread is actively executing spi_write() on another CPU, leading to hardware
bus timeouts?
[Severity: High]
Does this function also need to flush or cancel the asynchronous reset_work
before cutting device power?
If it doesn't synchronize with the workqueue, could there be a race where
spi_hid_reset_response() locklessly reads power_state as HIDSPI_ON, gets
preempted, and allows spi_hid_suspend() to power down the device? If the
workqueue then resumes and calls spi_hid_sync_request(), it would initiate
SPI transmissions to an unpowered device.
> + if (shid->power_state == HIDSPI_OFF)
> + return 0;
[Severity: High]
If this early return is taken, will the subsequent resume cause an unbalanced
IRQ enable warning?
In spi_hid_suspend(), this early return completely bypasses the call to
disable_irq(shid->spi->irq). However, upon system resume, spi_hid_resume()
unconditionally calls enable_irq(shid->spi->irq). If the IRQ disable depth
was not incremented during suspend, could this unconditional enable drop the
depth below zero and trigger a WARN(1, "Unbalanced enable for IRQ %d\n")?
> +
> + if (shid->hid) {
> + error = hid_driver_suspend(shid->hid, PMSG_SUSPEND);
> + if (error) {
> + dev_err(dev, "%s failed to suspend hid driver: %d\n",
> + __func__, error);
> + return error;
> + }
> + }
> +
> + disable_irq(shid->spi->irq);
> +
> + if (!device_may_wakeup(dev)) {
> + set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> + shid->ops->assert_reset(shid->ops);
> +
> + error = shid->ops->power_down(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power down\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + /* Undo partial suspend before returning error */
> + shid->ops->deassert_reset(shid->ops);
> + clear_bit(SPI_HID_RESET_PENDING, &shid->flags);
> + enable_irq(shid->spi->irq);
> + if (shid->hid)
> + hid_driver_reset_resume(shid->hid);
> + return error;
> + }
> +
> + shid->power_state = HIDSPI_OFF;
> + }
> + return 0;
> +}
> +
> +static int spi_hid_resume(struct spi_hid *shid)
> +{
> + int error;
> + struct device *dev = &shid->spi->dev;
> +
> + guard(mutex)(&shid->power_lock);
> +
> + if (!device_may_wakeup(dev)) {
> + if (shid->power_state == HIDSPI_OFF) {
> + shid->ops->assert_reset(shid->ops);
> +
> + shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> + error = shid->ops->power_up(shid->ops);
> + if (error) {
> + dev_err(dev, "%s: could not power up\n", __func__);
> + shid->regulator_error_count++;
> + shid->regulator_last_error = error;
> + return error;
> + }
> + shid->power_state = HIDSPI_ON;
> + shid->ops->deassert_reset(shid->ops);
> + }
> + }
> +
> + enable_irq(shid->spi->irq);
> +
> + if (shid->hid) {
> + error = hid_driver_reset_resume(shid->hid);
[Severity: High]
Does this code need to wait for the device's asynchronous RESET_RESPONSE
before sending further commands?
During a full resume cycle, the device is powered up, its reset line is
deasserted, and enable_irq() is called. The hardware then begins booting
and will signal readiness via an asynchronous RESET_RESPONSE interrupt.
However, spi_hid_resume() does not wait for this interrupt and synchronously
executes hid_driver_reset_resume() immediately. Because the SPI_HID_READY
flag is left set during suspend, any HID initialization commands generated
by the upper layer's resume callback will be immediately transmitted over
the SPI bus to a device that is still booting. Could this result in bus
timeouts and failed resume cycles?
> + if (error) {
> + dev_err(dev, "%s: failed to reset resume hid driver: %d\n",
> + __func__, error);
> + /* Undo partial resume before returning error */
> + disable_irq(shid->spi->irq);
> + if (!device_may_wakeup(dev)) {
> + set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> + shid->ops->assert_reset(shid->ops);
> + shid->ops->power_down(shid->ops);
> + shid->power_state = HIDSPI_OFF;
> + }
[Severity: High]
Can this error path create an unbalanced IRQ disable depth for
wakeup-capable devices?
If a device is wakeup-capable, spi_hid_suspend() skips powering down the
device (leaving power_state at HIDSPI_ON) but still calls disable_irq().
In spi_hid_resume(), enable_irq() is called, bringing the IRQ depth to 0.
If hid_driver_reset_resume() subsequently fails, this error recovery path
calls disable_irq() to undo the resume, but skips setting HIDSPI_OFF due
to the if (!device_may_wakeup(dev)) condition. Because the PM core ignores
resume errors and continues, the device remains at HIDSPI_ON.
On the next system suspend cycle, spi_hid_suspend() will see HIDSPI_ON and
execute disable_irq() a second time. A later successful resume will only call
enable_irq() once, leaving the device's interrupt permanently disabled.
> + return error;
> + }
> + }
> + return 0;
> +}
[ ... ]
> @@ -795,6 +887,11 @@ static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
> trace_spi_hid_header_transfer(shid);
>
> scoped_guard(mutex, &shid->io_lock) {
> + if (shid->power_state == HIDSPI_OFF) {
> + dev_warn(dev, "Device is off, ignoring interrupt\n");
> + goto out;
> + }
[Severity: Critical]
This isn't a bug introduced by this patch, but should this path return IRQ_NONE
instead of IRQ_HANDLED?
In spi_hid_error_handler(), if the device exceeds maximum reset attempts,
it is aborted and powered off (HIDSPI_OFF). The function then returns,
releasing guard(disable_irq)(&shid->spi->irq) and explicitly re-enabling
the IRQ line.
If the active-low interrupt line floats low due to power loss, it immediately
triggers. This threaded handler detects HIDSPI_OFF, logs a warning, and
returns IRQ_HANDLED via goto out.
Returning IRQ_HANDLED circumvents the kernel's spurious interrupt detector.
Because the physical line remains asserted and the handler claims success
without clearing the hardware source, the genirq subsystem immediately
re-fires the interrupt. Can this lead to a 100% CPU lockup?
[ ... ]
> @@ -1251,10 +1343,19 @@ int spi_hid_core_probe(struct spi_device *spi, struct spihid_ops *ops,
> dev_err(dev, "%s: unable to request threaded IRQ\n", __func__);
> return error;
> }
> + if (device_may_wakeup(dev)) {
> + error = dev_pm_set_wake_irq(dev, spi->irq);
> + if (error) {
> + dev_err(dev, "%s: failed to set wake IRQ\n", __func__);
> + return error;
> + }
> + }
>
> error = shid->ops->power_up(shid->ops);
> if (error) {
> dev_err(dev, "%s: could not power up\n", __func__);
> + if (device_may_wakeup(dev))
> + dev_pm_clear_wake_irq(dev);
[Severity: Medium]
Will this conditional cleanup leak memory if userspace alters the wakeup
settings?
In spi_hid_core_probe(), dev_pm_set_wake_irq() is executed if
device_may_wakeup(dev) is true. The cleanup path conditionally calls
dev_pm_clear_wake_irq(dev) based on the same check.
However, device_may_wakeup() evaluates dev->power.should_wakeup, which is
mutable state controllable by userspace via sysfs
(/sys/devices/.../power/wakeup). If userspace disables wakeup after the
device is probed, the check fails, bypassing the cleanup and permanently
leaking the allocated wake_irq memory while leaving a dangling pointer in
dev->power.wakeirq. Should dev_pm_clear_wake_irq() be called unconditionally
since it handles missing wake IRQs gracefully?
[ ... ]
> @@ -1284,9 +1385,31 @@ void spi_hid_core_remove(struct spi_device *spi)
> error = shid->ops->power_down(shid->ops);
> if (error)
> dev_err(dev, "failed to disable regulator\n");
> +
> + if (device_may_wakeup(dev))
> + dev_pm_clear_wake_irq(dev);
[Severity: Medium]
As noted above, should dev_pm_clear_wake_irq() be called unconditionally here
to avoid leaking the wake_irq structure if userspace disables device wakeup?
> }
> EXPORT_SYMBOL_GPL(spi_hid_core_remove);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=10
^ permalink raw reply
* Re: [PATCH v4 07/11] HID: spi_hid: add ACPI support for SPI over HID
From: sashiko-bot @ 2026-06-09 5:01 UTC (permalink / raw)
To: Jingyuan Liang; +Cc: robh, conor+dt, devicetree, dmitry.torokhov, linux-input
In-Reply-To: <20260609-send-upstream-v4-7-b843d5e6ced3@chromium.org>
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi_hid_acpi_deassert_reset() silently fails to execute the ACPI _RST method, breaking device initialization.
- [High] Use-After-Free due to flawed cleanup ordering: the reset workqueue is cancelled before the HID device is destroyed.
- [High] The shared buffer `shid->response` is read without holding `output_lock`, leading to data corruption if concurrent HID requests occur.
--
commit 8f475e9d4e137ed8f5267ef4406acd64e1baca61
Author: Angela Czubak <acz@semihalf.com>
Subject: HID: spi_hid: add ACPI support for SPI over HID
This commit detects and adds ACPI support for SPI HID devices.
> diff --git a/drivers/hid/spi-hid/spi-hid-acpi.c b/drivers/hid/spi-hid/spi-hid-acpi.c
> new file mode 100644
> index 0000000000000..298e3ba44d8a0
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-acpi.c
> @@ -0,0 +1,253 @@
[ ... ]
> +static int spi_hid_acpi_deassert_reset(struct spihid_ops *ops)
> +{
> + struct spi_hid_acpi_config *conf = container_of(ops,
> + struct spi_hid_acpi_config,
> + ops);
> +
> + return device_reset(&conf->adev->dev);
[Severity: High]
Does device_reset() work correctly here?
Since conf->adev->dev is the internal ACPI device structure without its fwnode
set, ACPI_HANDLE() will return null, causing __device_reset() to skip the ACPI
reset.
Also, looking at spi_hid_dev_init(), the return value of deassert_reset()
is ignored:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_init() {
...
shid->ops->deassert_reset(shid->ops);
...
}
Should it evaluate _RST directly or use &spi->dev instead, and should the
return value be checked to prevent hardware reset from silently failing?
> +}
[Severity: High]
Can the cleanup ordering lead to a use-after-free regression during removal?
In spi_hid_core_remove(), cancel_work_sync() is called before
spi_hid_stop_hid():
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_core_remove() {
...
cancel_work_sync(&shid->reset_work);
spi_hid_stop_hid(shid);
...
}
Since spi_hid_stop_hid() calls hid_destroy_device(), the HID device is still
accessible to userspace between the work cancellation and device destruction.
If a userspace process concurrently issues an ioctl during this window,
spi_hid_sync_request() will timeout, and the error handler in
spi_hid_get_request() could incorrectly schedule reset_work. Once removal
finishes and devm frees the shid structure, could the mistakenly scheduled
work execute and dereference the freed shid pointer?
Should spi_hid_stop_hid() be called before cancel_work_sync() to prevent this?
[Severity: High]
Can the shared buffer shid->response be read while corrupted if concurrent
HID requests occur?
In spi_hid_sync_request(), output_lock is acquired via scoped guard and is
implicitly released upon function return:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_sync_request() {
...
guard(mutex)(&shid->output_lock);
...
}
Functions like spi_hid_ll_raw_request() call into spi_hid_sync_request()
and then read from shid->response after the lock has been released:
drivers/hid/spi-hid/spi-hid-core.c:spi_hid_ll_raw_request() {
...
ret = spi_hid_sync_request(shid, ...);
...
ret = min_t(size_t, len,
(shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
buf[0] = shid->response->body[3];
memcpy(&buf[1], &shid->response->content, ret);
...
}
If multiple threads concurrently issue ioctls, could the second thread acquire
output_lock and overwrite shid->response with its own data before the first
thread has finished reading it?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=7
^ permalink raw reply
* Re: [PATCH v3] Input: elan_i2c - prevent division by zero on invalid device parameters
From: Dmitry Torokhov @ 2026-06-09 5:18 UTC (permalink / raw)
To: Ranjan Kumar; +Cc: bleung, bentiss, linux-input, linux-kernel
In-Reply-To: <20260518062624.1147959-1-kumarranja@chromium.org>
Hi Ranjan,
On Mon, May 18, 2026 at 06:26:24AM +0000, Ranjan Kumar wrote:
> The Elan I2C touchpad driver queries the device for its physical
> dimensions and trace counts to calculate the device resolution and width.
> However, if the device firmware or device tree provides invalid zero
> values for x_traces, y_traces, x_mm, or y_mm, it results in a fatal
> division-by-zero exception leading to a kernel panic during device probe.
>
> Add sanity checks to ensure these physical parameters are non-zero
> before performing the division. If invalid trace values are detected,
> log a warning and fall back to ETP_FWIDTH_REDUCE to prevent arithmetic
> underflow during touch reporting.
I would define some defaults that are not necessarily the same as
ETP_FWIDTH_REDUCE. The arithmetic overflow should be handled separately,
as it may still happen if we read (or set up via device tree) some small
(but non-zero) values.
> For invalid physical dimensions, fall
> back to a safe default of 1.
>
> This prevents the kernel panic while allowing the probe to complete
> successfully. Completing the probe ensures the sysfs nodes are created,
> keeping the firmware update path intact so a recovery firmware can be
> flashed to the device.
>
> Fixes: 6696777c6506 ("Input: add driver for Elan I2C/SMbus touchpad")
> Fixes: e3a9a1290688 ("Input: elan_i2c - do not query the info if they are provided")
> Signed-off-by: Ranjan Kumar <kumarranja@chromium.org>
> ---
> Changes in v3:
> - Changed trace fallback values from 1 to ETP_FWIDTH_REDUCE to prevent
> an unsigned integer underflow in elan_report_absolute().
> Changes in v2:
> - Changed error handling from aborting probe with -EINVAL to logging a
> warning and falling back to default values.
>
> drivers/input/mouse/elan_i2c_core.c | 25 +++++++++++++++++++++----
> 1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index fee1796da3d0..c64e1dd1e60b 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -425,8 +425,17 @@ static int elan_query_device_parameters(struct elan_tp_data *data)
> if (error)
> return error;
> }
> - data->width_x = data->max_x / x_traces;
> - data->width_y = data->max_y / y_traces;
> +
> + if (unlikely(x_traces == 0 || y_traces == 0)) {
I'd say "if (!x_traces || !y_traces) ...". This is not hot path so
annotating with unlikely does not buy us anything.
I wonder if we should be comparing with some threshold instead of 0.
Something above 90 I guess.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input: ipaq-micro-keys: simplify allocation
From: Dmitry Torokhov @ 2026-06-09 5:26 UTC (permalink / raw)
To: Rosen Penev; +Cc: linux-input, open list
In-Reply-To: <CAKxU2N8s8z5jD2JVU4b166jQYsUjWMj4Q5a-B6vkX7-fhCK8LQ@mail.gmail.com>
On Mon, Jun 08, 2026 at 01:20:36PM -0700, Rosen Penev wrote:
> On Mon, Jun 8, 2026 at 9:49 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Hi Rosen,
> >
> > On Sun, Jun 07, 2026 at 09:50:41PM -0700, Rosen Penev wrote:
> > > Use a flexible array member to have a single allocation.
> >
> > Why does it have to be flexible? The size is known.
> From a quick glance, that would be a compilation error. micro_keycodes
> gets declared after ipaq_micro_keys
We could swap them around, no big deal.
>
> From looking at a bunch of struct definitions, [ARRAY_SIZE(foo)] seems
> to be atypical.
There are a lot of "#define BLAH_SIZE ARRAY_SIZE(blah)" that later
used in array definitions.
Thanks.
--
Dmitry
^ permalink raw reply
* [syzbot] [usb?] [input?] KMSAN: uninit-value in steam_register
From: syzbot @ 2026-06-09 8:35 UTC (permalink / raw)
To: linux-input, linux-kernel, linux-usb, syzkaller-bugs
Hello,
syzbot found the following issue on:
HEAD commit: 8e65320d91cd Merge tag 'drm-fixes-2026-06-06' of https://g..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13cb9e7a580000
kernel config: https://syzkaller.appspot.com/x/.config?x=a0ca3b8cb3875012
dashboard link: https://syzkaller.appspot.com/bug?extid=75f3f9bff8c510602d36
compiler: Debian clang version 21.1.8 (++20251221033036+2078da43e25a-1~exp1~20251221153213.50), Debian LLD 21.1.8
Unfortunately, I don't have any reproducer for this issue yet.
Downloadable assets:
disk image: https://storage.googleapis.com/syzbot-assets/793bd3b9425c/disk-8e65320d.raw.xz
vmlinux: https://storage.googleapis.com/syzbot-assets/4bfe5cfc266c/vmlinux-8e65320d.xz
kernel image: https://storage.googleapis.com/syzbot-assets/99c7cd705a06/bzImage-8e65320d.xz
IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+75f3f9bff8c510602d36@syzkaller.appspotmail.com
hid-steam 0003:28DE:1102.0007: unknown main item tag 0x0
hid-steam 0003:28DE:1102.0007: unknown main item tag 0x0
hid-steam 0003:28DE:1102.0007: : USB HID v7f.fd Device [HID 28de:1102] on usb-dummy_hcd.1-1/input0
=====================================================
BUG: KMSAN: uninit-value in steam_get_serial drivers/hid/hid-steam.c:457 [inline]
BUG: KMSAN: uninit-value in steam_register+0xd83/0x10e0 drivers/hid/hid-steam.c:965
steam_get_serial drivers/hid/hid-steam.c:457 [inline]
steam_register+0xd83/0x10e0 drivers/hid/hid-steam.c:965
steam_probe+0x6f1/0x19b0 drivers/hid/hid-steam.c:1273
__hid_device_probe drivers/hid/hid-core.c:2822 [inline]
hid_device_probe+0x60d/0xb90 drivers/hid/hid-core.c:2859
call_driver_probe drivers/base/dd.c:-1 [inline]
really_probe+0x4d5/0xe40 drivers/base/dd.c:709
__driver_probe_device+0x2fb/0x410 drivers/base/dd.c:871
driver_probe_device+0x70/0x8f0 drivers/base/dd.c:901
__device_attach_driver+0x4ee/0x950 drivers/base/dd.c:1029
bus_for_each_drv+0x3e3/0x680 drivers/base/bus.c:500
__device_attach+0x3c5/0x5f0 drivers/base/dd.c:1101
device_initial_probe+0x126/0x170 drivers/base/dd.c:1156
bus_probe_device+0x287/0x530 drivers/base/bus.c:613
device_add+0x12e3/0x1c90 drivers/base/core.c:3706
hid_add_device+0x440/0x5d0 drivers/hid/hid-core.c:3003
usbhid_probe+0x1a25/0x20e0 drivers/hid/usbhid/hid-core.c:1449
usb_probe_interface+0xd51/0x1440 drivers/usb/core/driver.c:396
call_driver_probe drivers/base/dd.c:-1 [inline]
really_probe+0x4d5/0xe40 drivers/base/dd.c:709
__driver_probe_device+0x2fb/0x410 drivers/base/dd.c:871
driver_probe_device+0x70/0x8f0 drivers/base/dd.c:901
__device_attach_driver+0x4ee/0x950 drivers/base/dd.c:1029
bus_for_each_drv+0x3e3/0x680 drivers/base/bus.c:500
__device_attach+0x3c5/0x5f0 drivers/base/dd.c:1101
device_initial_probe+0x126/0x170 drivers/base/dd.c:1156
bus_probe_device+0x287/0x530 drivers/base/bus.c:613
device_add+0x12e3/0x1c90 drivers/base/core.c:3706
usb_set_configuration+0x3484/0x3b60 drivers/usb/core/message.c:2268
usb_generic_driver_probe+0xfc/0x290 drivers/usb/core/generic.c:250
usb_probe_device+0x38d/0x690 drivers/usb/core/driver.c:291
call_driver_probe drivers/base/dd.c:-1 [inline]
really_probe+0x4d5/0xe40 drivers/base/dd.c:709
__driver_probe_device+0x2fb/0x410 drivers/base/dd.c:871
driver_probe_device+0x70/0x8f0 drivers/base/dd.c:901
__device_attach_driver+0x4ee/0x950 drivers/base/dd.c:1029
bus_for_each_drv+0x3e3/0x680 drivers/base/bus.c:500
__device_attach+0x3c5/0x5f0 drivers/base/dd.c:1101
device_initial_probe+0x126/0x170 drivers/base/dd.c:1156
bus_probe_device+0x287/0x530 drivers/base/bus.c:613
device_add+0x12e3/0x1c90 drivers/base/core.c:3706
usb_new_device+0x106a/0x2120 drivers/usb/core/hub.c:2695
hub_port_connect drivers/usb/core/hub.c:5567 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5707 [inline]
port_event drivers/usb/core/hub.c:5871 [inline]
hub_event+0x5404/0x77c0 drivers/usb/core/hub.c:5953
process_one_work kernel/workqueue.c:3314 [inline]
process_scheduled_works+0xb65/0x1e40 kernel/workqueue.c:3397
worker_thread+0xee4/0x1590 kernel/workqueue.c:3478
kthread+0x53a/0x5f0 kernel/kthread.c:436
ret_from_fork+0x20f/0x8d0 arch/x86/kernel/process.c:158
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:245
Local variable reply.i created at:
steam_get_serial drivers/hid/hid-steam.c:448 [inline]
steam_register+0x180/0x10e0 drivers/hid/hid-steam.c:965
steam_probe+0x6f1/0x19b0 drivers/hid/hid-steam.c:1273
CPU: 0 UID: 0 PID: 7403 Comm: kworker/0:6 Tainted: G L syzkaller #0 PREEMPT(lazy)
Tainted: [L]=SOFTLOCKUP
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 04/18/2026
Workqueue: usb_hub_wq hub_event
=====================================================
---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
If the report is already addressed, let syzbot know by replying with:
#syz fix: exact-commit-title
If you want to overwrite report's subsystems, reply with:
#syz set subsystems: new-subsystem
(See the list of subsystem names on the web dashboard)
If the report is a duplicate of another one, reply with:
#syz dup: exact-subject-of-another-report
If you want to undo deduplication, reply with:
#syz undup
^ permalink raw reply
* Re: [PATCH v3 2/2] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: Konrad Dybcio @ 2026-06-09 9:30 UTC (permalink / raw)
To: Harrison Vanderbyl, linux-kernel, linux-input, linux-arm-msm,
devicetree
Cc: jikos, bentiss, andersson, konradybcio, robh, krzk+dt, conor+dt,
dmitry.baryshkov
In-Reply-To: <20260529011619.9586-3-harrison.vanderbyl@gmail.com>
On 5/29/26 3:16 AM, Harrison Vanderbyl wrote:
> Initial device tree for Microsoft Surface Pro 12in
>
> Currently supported:
> - UFS
> - Touchscreen
> - Pen
> - USB 3.2 x2 (DP Alt Mode)
> - Audio
> - Wifi
> - Bluetooth
> - CDSP
> - ADSP
> - GPU
>
> Not currently supported:
> - Accelerometer
> - Front, Back and IR cameras
> - IRIS video decoder
>
> Tested on Surface_Pro_12in_1st_Ed_with_Snapdragon_2110
>
> Signed-off-by: Harrison Vanderbyl <harrison.vanderbyl@gmail.com>
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
> ---
lg, just a couple nits
[...]
> + codec {
> + sound-dai = <&left_spkr>, <&right_spkr>,
> + <&swr0 0>, <&lpass_wsamacro 0>;
Please align the <s
[...]
> +&i2c9 {
> + clock-frequency = <400000>;
> +
> + /* NFC @28, commercial devices only */
> +
> + status = "okay";
Because we'd put the subnodes for devices under the 'status' line (DTC
enforces that), please move the comment below as well
Or since it's @28, perhaps it's just nxp,nxp-nci-i2c and it could work
for you ootb?
[...]
> +&tlmm {
> + gpio-reserved-ranges = <34 2>, /* Unused */
> + <44 4>; /* SPI (TPM) */
Please align the <s (or is it my mail client playing tricks on me?)
Konrad
^ permalink raw reply
* Re: [PATCH v3] HID: i2c-hid: Refactor _DSM helper and add i2c-hid-acpi-prp0001 driver
From: 谢致邦 (XIE Zhibang) @ 2026-06-09 10:10 UTC (permalink / raw)
To: linux-input, hansg, dmitry.torokhov, bentiss
Cc: Yeking, dianders, jikos, linux-kernel, sashiko-bot,
sashiko-reviews, superm1
In-Reply-To: <4243c594-6535-4111-86cb-69784b0a34b7@kernel.org>
Hi Hans,
Thanks for the review.
The current header-based approach lets i2c-hid-acpi-prp0001.c work
independently by just including a header for the _DSM helper. Switching
to EXPORT_SYMBOL_GPL would force prp0001 to depend on i2c-hid-acpi —
a Kconfig "depends on I2C_HID_ACPI", plus module-level dependency at
load time — all for sharing a single function. I find that hard to
accept.
I understand that adding a new header for one prototype is unusual,
though it contains only a single static inline function and introduces
no runtime dependency between the two drivers.
Looking back at the full discussion: Benjamin made it clear from the
start that he doesn't want i2c-hid-of.c handling ACPI _DSM fallback.
His line is that ACPI devices and OF devices should each go through
their own drivers without cross-contamination. If you still prefer
exporting the function from i2c-hid-acpi.c, then prp0001 would have to
drag i2c-hid-acpi along with it — an independent leaf driver turned
into something that can't stand alone.
If you still prefer the export approach, then I'd rather take a simpler
approach: add an of_match directly to
i2c-hid-acpi.c, and return -ENODEV silently at the top of probe if
hid-descr-addr is present. Same logic as the prp0001 driver, but one
fewer file, no module dependency, no extra Kconfig entry, and no OF
driver touching ACPI devices — which should also satisfy Benjamin's
concern.
Regards,
Zhibang
^ permalink raw reply
* Re: [PATCH v3] HID: i2c-hid: Refactor _DSM helper and add i2c-hid-acpi-prp0001 driver
From: 谢致邦 (XIE Zhibang) @ 2026-06-09 10:21 UTC (permalink / raw)
To: linux-input, hansg, dmitry.torokhov, bentiss
Cc: Yeking, dianders, jikos, linux-kernel, sashiko-bot,
sashiko-reviews, superm1
In-Reply-To: <tencent_C2BEF7C0584BFE713DC7B41FFD7007B6D30A@qq.com>
Hi Hans,
Thanks for the review.
The current header-based approach lets i2c-hid-acpi-prp0001.c work
independently by just including a header for the _DSM helper. Switching
to EXPORT_SYMBOL_GPL would force prp0001 to depend on i2c-hid-acpi —
a Kconfig "depends on I2C_HID_ACPI", plus module-level dependency at
load time — all for sharing a single function. I find that hard to
accept.
I understand that adding a new header for one function is unusual,
though it contains only a single static inline and introduces no runtime
dependency between the two drivers.
Looking back at the full discussion: Benjamin made it clear from the
start that he doesn't want i2c-hid-of.c handling ACPI _DSM fallback.
His line is that ACPI devices and OF devices should each go through
their own drivers without cross-contamination. If you still prefer
exporting the function from i2c-hid-acpi.c, then prp0001 would have to
drag i2c-hid-acpi along with it — an independent leaf driver turned
into something that can't stand alone.
If you still prefer the export approach, then I'd rather take a simpler
approach: add an of_match directly to i2c-hid-acpi.c, and return
-ENODEV silently at the top of probe if hid-descr-addr is present. Same
logic as the prp0001 driver, but one fewer file, no module dependency,
no extra Kconfig entry, and no OF driver touching ACPI devices — which
should also satisfy Benjamin's concern.
[corrected: s/prototype/function/ — the header contains a definition,
not just a declaration]
Regards,
Zhibang
^ permalink raw reply
* [PATCH v4] HID: i2c-hid: Refactor _DSM helper and add i2c-hid-acpi-prp0001 driver
From: 谢致邦 (XIE Zhibang) @ 2026-06-09 10:54 UTC (permalink / raw)
To: linux-input, hansg, dmitry.torokhov, bentiss, yeking
Cc: dianders, jikos, linux-kernel, sashiko-bot, sashiko-reviews,
superm1, 谢致邦 (XIE Zhibang)
In-Reply-To: <tencent_45D6247F04CF37ED6A3F858178B4303D6808@qq.com>
Move the _DSM call that gets the HID descriptor address from
i2c-hid-acpi.c into i2c-hid-acpi.h as a static inline so both the ACPI
and the new PRP0001 driver can use it. While refactoring, move the
blacklist check and the _DSM call to the top of probe() to avoid a
pointless alloc when the device is blacklisted or does not implement the
_DSM.
Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3,
are declared with _HID "PRP0001" and _DSD compatible "hid-over-i2c" but
lack "hid-descr-addr" from the _DSD and provide the HID descriptor
address only through an ACPI _DSM. The OF driver fails to probe them
because it requires hid-descr-addr. Add a new driver that handles these
devices by calling the shared _DSM helper.
Fixes: b33752c30023 ("HID: i2c-hid: Reorganize so ACPI and OF are separate modules")
Signed-off-by: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
---
v2: Name the unused parameter and document why
acpi_device_fix_up_power() is skipped.
v3: Add a dev_warn() asking users to contact vendors for firmware
updates, and use existing locals in devm_kzalloc() and
acpi_device_fix_up_power().
v4: Double the power-up delay from 250ms to 500ms.
drivers/hid/i2c-hid/Kconfig | 18 ++++
drivers/hid/i2c-hid/Makefile | 1 +
drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c | 98 ++++++++++++++++++++++
drivers/hid/i2c-hid/i2c-hid-acpi.c | 52 ++++--------
drivers/hid/i2c-hid/i2c-hid-acpi.h | 32 +++++++
5 files changed, 163 insertions(+), 38 deletions(-)
create mode 100644 drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c
create mode 100644 drivers/hid/i2c-hid/i2c-hid-acpi.h
diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index e8d51f410cc1..7db8b2abff78 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -22,6 +22,24 @@ config I2C_HID_ACPI
will be called i2c-hid-acpi. It will also build/depend on the
module i2c-hid.
+config I2C_HID_ACPI_PRP0001
+ tristate "Driver for PRP0001 devices missing hid-descr-addr"
+ depends on ACPI
+ depends on DRM || !DRM
+ select I2C_HID_CORE
+ help
+ Say Y here if you want support for I2C HID touchpads that
+ are declared with _HID "PRP0001" and _DSD compatible
+ "hid-over-i2c" but lack the "hid-descr-addr" property from
+ the _DSD. The HID descriptor address is instead provided
+ through an ACPI _DSM. Known affected devices include the
+ Lenovo KaiTian N60d and Inspur CP300L3.
+
+ If unsure, say N.
+
+ This support is also available as a module. If so, the
+ module will be called i2c-hid-acpi-prp0001.
+
config I2C_HID_OF
tristate "HID over I2C transport layer Open Firmware driver"
# No "depends on OF" because this can also be used for manually
diff --git a/drivers/hid/i2c-hid/Makefile b/drivers/hid/i2c-hid/Makefile
index 55bd5e0f35af..da420c1a8ec6 100644
--- a/drivers/hid/i2c-hid/Makefile
+++ b/drivers/hid/i2c-hid/Makefile
@@ -9,6 +9,7 @@ i2c-hid-objs = i2c-hid-core.o
i2c-hid-$(CONFIG_DMI) += i2c-hid-dmi-quirks.o
obj-$(CONFIG_I2C_HID_ACPI) += i2c-hid-acpi.o
+obj-$(CONFIG_I2C_HID_ACPI_PRP0001) += i2c-hid-acpi-prp0001.o
obj-$(CONFIG_I2C_HID_OF) += i2c-hid-of.o
obj-$(CONFIG_I2C_HID_OF_ELAN) += i2c-hid-of-elan.o
obj-$(CONFIG_I2C_HID_OF_GOODIX) += i2c-hid-of-goodix.o
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c b/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c
new file mode 100644
index 000000000000..a65830279f63
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * HID over I2C driver for PRP0001 devices missing hid-descr-addr
+ *
+ * Some devices, for example the Lenovo KaiTian N60d and Inspur CP300L3, use
+ * _HID "PRP0001" with _DSD compatible "hid-over-i2c" but lack "hid-descr-addr"
+ * from the _DSD. The HID descriptor address is provided only through an ACPI
+ * _DSM. The TPD0 node in the DSDT shows _DSM Function 1 returning 0x20.
+ *
+ * Copyright (C) 2026 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+#include "i2c-hid.h"
+#include "i2c-hid-acpi.h"
+
+static int i2c_hid_acpi_prp0001_power_up(struct i2chid_ops *ops)
+{
+ /* give the device time to power up */
+ msleep(500);
+ return 0;
+}
+
+static struct i2chid_ops i2c_hid_acpi_prp0001_ops = {
+ .power_up = i2c_hid_acpi_prp0001_power_up,
+ /*
+ * No .restore_sequence needed: the _DSM on these devices returns a
+ * constant (0x20) with no side effects, unlike some PNP0C50 _DSM
+ * implementations that switch the hardware between PS/2 and I2C modes.
+ */
+};
+
+static int i2c_hid_acpi_prp0001_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ struct acpi_device *adev;
+ u16 hid_descriptor_address;
+ int ret;
+
+ /* If hid-descr-addr is present, let i2c-hid-of handle it */
+ if (device_property_present(dev, "hid-descr-addr"))
+ return -ENODEV;
+
+ adev = ACPI_COMPANION(dev);
+ if (!adev)
+ return -ENODEV;
+
+ ret = i2c_hid_acpi_get_descriptor(adev);
+ if (ret < 0)
+ return ret;
+ dev_warn(dev,
+ "hid-descr-addr device property NOT found, using ACPI _DSM fallback. Contact vendor for firmware update!\n");
+ hid_descriptor_address = ret;
+
+ /*
+ * No acpi_device_fix_up_power() needed: TPD0 has no _PS0, _PS3, _PSC
+ * or _PRx methods and follows I2C bus power.
+ */
+ return i2c_hid_core_probe(client, &i2c_hid_acpi_prp0001_ops,
+ hid_descriptor_address, 0);
+}
+
+static const struct of_device_id i2c_hid_acpi_prp0001_of_match[] = {
+ { .compatible = "hid-over-i2c" },
+ {},
+};
+MODULE_DEVICE_TABLE(of, i2c_hid_acpi_prp0001_of_match);
+
+static const struct i2c_device_id i2c_hid_acpi_prp0001_id[] = {
+ { .name = "hid-over-i2c" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_prp0001_id);
+
+static struct i2c_driver i2c_hid_acpi_prp0001_driver = {
+ .driver = {
+ .name = "i2c_hid_acpi_prp0001",
+ .pm = &i2c_hid_core_pm,
+ .probe_type = PROBE_PREFER_ASYNCHRONOUS,
+ .of_match_table = of_match_ptr(i2c_hid_acpi_prp0001_of_match),
+ },
+
+ .probe = i2c_hid_acpi_prp0001_probe,
+ .remove = i2c_hid_core_remove,
+ .shutdown = i2c_hid_core_shutdown,
+ .id_table = i2c_hid_acpi_prp0001_id,
+};
+
+module_i2c_driver(i2c_hid_acpi_prp0001_driver);
+
+MODULE_DESCRIPTION("HID over I2C driver for PRP0001 devices missing hid-descr-addr");
+MODULE_AUTHOR("谢致邦 (XIE Zhibang) <Yeking@Red54.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.c b/drivers/hid/i2c-hid/i2c-hid-acpi.c
index abd700a101f4..13f977d6aab6 100644
--- a/drivers/hid/i2c-hid/i2c-hid-acpi.c
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.c
@@ -25,9 +25,9 @@
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/pm.h>
-#include <linux/uuid.h>
#include "i2c-hid.h"
+#include "i2c-hid-acpi.h"
struct i2c_hid_acpi {
struct i2chid_ops ops;
@@ -48,39 +48,11 @@ static const struct acpi_device_id i2c_hid_acpi_blacklist[] = {
{ }
};
-/* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
-static guid_t i2c_hid_guid =
- GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
- 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
-
-static int i2c_hid_acpi_get_descriptor(struct i2c_hid_acpi *ihid_acpi)
-{
- struct acpi_device *adev = ihid_acpi->adev;
- acpi_handle handle = acpi_device_handle(adev);
- union acpi_object *obj;
- u16 hid_descriptor_address;
-
- if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
- return -ENODEV;
-
- obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid, 1, 1, NULL,
- ACPI_TYPE_INTEGER);
- if (!obj) {
- acpi_handle_err(handle, "Error _DSM call to get HID descriptor address failed\n");
- return -ENODEV;
- }
-
- hid_descriptor_address = obj->integer.value;
- ACPI_FREE(obj);
-
- return hid_descriptor_address;
-}
-
static void i2c_hid_acpi_restore_sequence(struct i2chid_ops *ops)
{
struct i2c_hid_acpi *ihid_acpi = container_of(ops, struct i2c_hid_acpi, ops);
- i2c_hid_acpi_get_descriptor(ihid_acpi);
+ i2c_hid_acpi_get_descriptor(ihid_acpi->adev);
}
static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
@@ -93,24 +65,28 @@ static void i2c_hid_acpi_shutdown_tail(struct i2chid_ops *ops)
static int i2c_hid_acpi_probe(struct i2c_client *client)
{
struct device *dev = &client->dev;
+ struct acpi_device *adev = ACPI_COMPANION(dev);
struct i2c_hid_acpi *ihid_acpi;
u16 hid_descriptor_address;
int ret;
- ihid_acpi = devm_kzalloc(&client->dev, sizeof(*ihid_acpi), GFP_KERNEL);
+ if (acpi_match_device_ids(adev, i2c_hid_acpi_blacklist) == 0)
+ return -ENODEV;
+
+ ret = i2c_hid_acpi_get_descriptor(adev);
+ if (ret < 0)
+ return ret;
+ hid_descriptor_address = ret;
+
+ ihid_acpi = devm_kzalloc(dev, sizeof(*ihid_acpi), GFP_KERNEL);
if (!ihid_acpi)
return -ENOMEM;
- ihid_acpi->adev = ACPI_COMPANION(dev);
+ ihid_acpi->adev = adev;
ihid_acpi->ops.shutdown_tail = i2c_hid_acpi_shutdown_tail;
ihid_acpi->ops.restore_sequence = i2c_hid_acpi_restore_sequence;
- ret = i2c_hid_acpi_get_descriptor(ihid_acpi);
- if (ret < 0)
- return ret;
- hid_descriptor_address = ret;
-
- acpi_device_fix_up_power(ihid_acpi->adev);
+ acpi_device_fix_up_power(adev);
return i2c_hid_core_probe(client, &ihid_acpi->ops,
hid_descriptor_address, 0);
diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi.h b/drivers/hid/i2c-hid/i2c-hid-acpi.h
new file mode 100644
index 000000000000..8cebbeebcc23
--- /dev/null
+++ b/drivers/hid/i2c-hid/i2c-hid-acpi.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef _I2C_HID_ACPI_H
+#define _I2C_HID_ACPI_H
+
+#include <linux/acpi.h>
+#include <linux/uuid.h>
+
+static inline int i2c_hid_acpi_get_descriptor(struct acpi_device *adev)
+{
+ /* HID I²C Device: 3cdff6f7-4267-4555-ad05-b30a3d8938de */
+ static const guid_t i2c_hid_guid =
+ GUID_INIT(0x3CDFF6F7, 0x4267, 0x4555,
+ 0xAD, 0x05, 0xB3, 0x0A, 0x3D, 0x89, 0x38, 0xDE);
+
+ acpi_handle handle = acpi_device_handle(adev);
+ union acpi_object *obj;
+ u16 addr;
+
+ obj = acpi_evaluate_dsm_typed(handle, &i2c_hid_guid,
+ 1, 1, NULL, ACPI_TYPE_INTEGER);
+ if (!obj) {
+ acpi_handle_err(handle, "Error _DSM call to get HID descriptor address failed\n");
+ return -ENODEV;
+ }
+
+ addr = obj->integer.value;
+ ACPI_FREE(obj);
+ return addr;
+}
+
+#endif
--
2.52.0
^ permalink raw reply related
* Re: [PATCH v3 2/2] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: Harrison Vanderbyl @ 2026-06-09 11:04 UTC (permalink / raw)
To: konrad.dybcio
Cc: andersson, bentiss, conor+dt, devicetree, dmitry.baryshkov,
harrison.vanderbyl, jikos, konradybcio, krzk+dt, linux-arm-msm,
linux-input, linux-kernel, robh
In-Reply-To: <4ced3207-a132-4ec5-a0eb-79f75f5cb4bb@oss.qualcomm.com>
On 6/9/26 11:30 AM, Konrad Dybcio wrote:
> Because we'd put the subnodes for devices under the 'status' line (DTC
> enforces that), please move the comment below as well
Will do,
> Or since it's @28, perhaps it's just nxp,nxp-nci-i2c and it could work
> for you ootb?
Unfortunately for me, the device/antenna is not present on the consumer version,
I was able to confirm this during a speaker replacement.
> Please align the <s (or is it my mail client playing tricks on me?)
The alignment changes depending on what text editor I copy paste this section into.
I have them aligned the same as the other device trees are aligned though.
^ permalink raw reply
* Re: [PATCH v4] HID: i2c-hid: Refactor _DSM helper and add i2c-hid-acpi-prp0001 driver
From: sashiko-bot @ 2026-06-09 11:06 UTC (permalink / raw)
Cc: dmitry.torokhov, linux-input
In-Reply-To: <tencent_6169D37BDAAD7FFEEC9E41DA197FE6028309@qq.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Using `of_match_ptr()` on the `of_match_table` prevents native ACPI PRP0001 matching when `CONFIG_OF` is disabled.
--
commit fdffe866c804a579c14dbd7b1c8529b20466017b
Author: 谢致邦 (XIE Zhibang) <Yeking@Red54.com>
HID: i2c-hid: Refactor _DSM helper and add i2c-hid-acpi-prp0001 driver
This commit moves the _DSM call that retrieves the HID descriptor address into
a shared header. It also introduces the i2c-hid-acpi-prp0001 driver to handle
devices with _HID "PRP0001" and _DSD compatible "hid-over-i2c" that are missing
the "hid-descr-addr" property.
> diff --git a/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c b/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/hid/i2c-hid/i2c-hid-acpi-prp0001.c
[ ... ]
> +static const struct of_device_id i2c_hid_acpi_prp0001_of_match[] = {
> + { .compatible = "hid-over-i2c" },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_hid_acpi_prp0001_of_match);
> +
[ ... ]
> +static struct i2c_driver i2c_hid_acpi_prp0001_driver = {
> + .driver = {
> + .name = "i2c_hid_acpi_prp0001",
> + .pm = &i2c_hid_core_pm,
> + .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> + .of_match_table = of_match_ptr(i2c_hid_acpi_prp0001_of_match),
[Severity: High]
Will this prevent the driver from probing on ACPI systems that do not enable
CONFIG_OF?
The driver relies on ACPI PRP0001 matching to match the _DSD compatible
property against the of_match_table. However, if CONFIG_OF is disabled
(which is standard for x86 ACPI platforms where this hardware exists),
of_match_ptr() evaluates to NULL.
When of_match_table is NULL, the ACPI driver core will silently fail to
match the device, leaving the touchpad non-functional. Should we drop the
of_match_ptr() wrapper here to ensure the table is always available for
PRP0001 matching?
> + },
> +
> + .probe = i2c_hid_acpi_prp0001_probe,
> + .remove = i2c_hid_core_remove,
> + .shutdown = i2c_hid_core_shutdown,
> + .id_table = i2c_hid_acpi_prp0001_id,
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/tencent_6169D37BDAAD7FFEEC9E41DA197FE6028309@qq.com?part=1
^ permalink raw reply
* Re: [PATCH v3 2/2] arm64: dts: qcom: Add Microsoft Surface Pro 12in
From: Konrad Dybcio @ 2026-06-09 12:06 UTC (permalink / raw)
To: Harrison Vanderbyl
Cc: andersson, bentiss, conor+dt, devicetree, dmitry.baryshkov, jikos,
konradybcio, krzk+dt, linux-arm-msm, linux-input, linux-kernel,
robh
In-Reply-To: <20260609110429.11427-1-harrison.vanderbyl@gmail.com>
On 6/9/26 1:04 PM, Harrison Vanderbyl wrote:
> On 6/9/26 11:30 AM, Konrad Dybcio wrote:
>> Because we'd put the subnodes for devices under the 'status' line (DTC
>> enforces that), please move the comment below as well
>
> Will do,
>
>> Or since it's @28, perhaps it's just nxp,nxp-nci-i2c and it could work
>> for you ootb?
>
> Unfortunately for me, the device/antenna is not present on the consumer version,
> I was able to confirm this during a speaker replacement.
So perhaps "commercial devices only" refers to ""Surface Pro for Business
12-inch"" [1] which is presumably an actual separate SKU?
I initially interpreted "commercial devices" as "not prototypes"
[1] https://cdn-dynmedia-1.microsoft.com/is/content/microsoftcorp/microsoft/mlsd/documents/presentations/en-us/surface/MSFT-echo-surface-pro-12-inch-snapdragon-techspecs-factsheet.pdf
>
>> Please align the <s (or is it my mail client playing tricks on me?)
>
> The alignment changes depending on what text editor I copy paste this section into.
This shouldn't be the case, so long as you set the kernel-standard tab
width to 8.
Konrad
^ permalink raw reply
* [PATCH v3 1/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad
From: Lee Jones @ 2026-06-09 12:13 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
wacom_intuos_pad() accesses wacom->shared->touch_input locklessly
inside the interrupt handler context. If the Touch sibling device
is disconnected, wacom_remove_shared_data() clears 'touch_input'
outside any lock, creating a Time-of-Check to Time-of-Use (TOCTOU)
race condition where a preempted reader in interrupt context
dereferences the freed pointer, leading to a Use-After-Free.
Resolve this by introducing RCU protection for the touch_input
pointer:
- Annotate 'touch_input' in wacom_shared struct with __rcu
- Wrap all lockless readers in wacom_wac.c with rcu_read_lock() and
rcu_dereference() using a unified wacom_report_touch_mute()
helper
- Update writers in wacom_sys.c using rcu_assign_pointer()
- Call synchronize_rcu() in wacom_remove_shared_data() to ensure
all active RCU readers have finished before the input device is
freed
Also wrap wacom_set_shared_values() and touch/pen assignments in
wacom_add_shared_data() inside the wacom_udev_list_lock to serialize
concurrent probe assignments, and verify that 'shared->touch == hdev'
before setting touch_input to prevent concurrent sibling probe state
desynchronization.
Finally, advertise the SW_MUTE_DEVICE capability on Touch input
devices prior to registration in wacom_setup_touch_input_capabilities()
to prevent invalid post-registration capability modifications.
Fixes: 961794a00eab ("Input: wacom - add reporting of SW_MUTE_DEVICE events")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
drivers/hid/wacom_sys.c | 41 ++++++++++++++----------
drivers/hid/wacom_wac.c | 70 ++++++++++++++++++++++-------------------
drivers/hid/wacom_wac.h | 2 +-
3 files changed, 63 insertions(+), 50 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 2220168bf116..7ba589826548 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -877,10 +877,16 @@ static void wacom_remove_shared_data(void *res)
data = container_of(wacom_wac->shared, struct wacom_hdev_data,
shared);
- if (wacom_wac->shared->touch == wacom->hdev)
- wacom_wac->shared->touch = NULL;
- else if (wacom_wac->shared->pen == wacom->hdev)
- wacom_wac->shared->pen = NULL;
+ scoped_guard(mutex, &wacom_udev_list_lock) {
+ if (wacom_wac->shared->touch == wacom->hdev) {
+ wacom_wac->shared->touch = NULL;
+ rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
+ } else if (wacom_wac->shared->pen == wacom->hdev) {
+ wacom_wac->shared->pen = NULL;
+ }
+ }
+
+ synchronize_rcu();
kref_put(&data->kref, wacom_release_shared_data);
wacom_wac->shared = NULL;
@@ -909,6 +915,11 @@ static int wacom_add_shared_data(struct hid_device *hdev)
list_add_tail(&data->list, &wacom_udev_list);
}
+ if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
+ data->shared.touch = hdev;
+ else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
+ data->shared.pen = hdev;
+
mutex_unlock(&wacom_udev_list_lock);
wacom_wac->shared = &data->shared;
@@ -917,11 +928,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
if (retval)
return retval;
- if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
- wacom_wac->shared->touch = hdev;
- else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
- wacom_wac->shared->pen = hdev;
-
return retval;
}
@@ -2345,9 +2351,15 @@ static void wacom_release_resources(struct wacom *wacom)
static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
{
+ struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+
+ mutex_lock(&wacom_udev_list_lock);
+
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
- wacom_wac->shared->type = wacom_wac->features.type;
- wacom_wac->shared->touch_input = wacom_wac->touch_input;
+ if (wacom_wac->shared->touch == wacom->hdev) {
+ wacom_wac->shared->type = wacom_wac->features.type;
+ rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
+ }
}
if (wacom_wac->has_mute_touch_switch) {
@@ -2361,12 +2373,7 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
wacom_wac->shared->is_touch_on = true;
}
- if (wacom_wac->shared->has_mute_touch_switch &&
- wacom_wac->shared->touch_input) {
- set_bit(EV_SW, wacom_wac->shared->touch_input->evbit);
- input_set_capability(wacom_wac->shared->touch_input, EV_SW,
- SW_MUTE_DEVICE);
- }
+ mutex_unlock(&wacom_udev_list_lock);
}
static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index da1f0ea85625..495960227b8d 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -510,6 +510,22 @@ static void wacom_intuos_schedule_prox_event(struct wacom_wac *wacom_wac)
}
}
+static void wacom_report_touch_mute(struct wacom_wac *wacom_wac, bool mute)
+{
+ struct input_dev *touch_input;
+
+ if (!wacom_wac->shared)
+ return;
+
+ rcu_read_lock();
+ touch_input = rcu_dereference(wacom_wac->shared->touch_input);
+ if (touch_input) {
+ input_report_switch(touch_input, SW_MUTE_DEVICE, mute);
+ input_sync(touch_input);
+ }
+ rcu_read_unlock();
+}
+
static int wacom_intuos_pad(struct wacom_wac *wacom)
{
struct wacom_features *features = &wacom->features;
@@ -650,12 +666,8 @@ static int wacom_intuos_pad(struct wacom_wac *wacom)
input_report_key(input, KEY_CONTROLPANEL, menu);
input_report_key(input, KEY_INFO, info);
- if (wacom->shared && wacom->shared->touch_input) {
- input_report_switch(wacom->shared->touch_input,
- SW_MUTE_DEVICE,
- !wacom->shared->is_touch_on);
- input_sync(wacom->shared->touch_input);
- }
+ if (wacom->shared)
+ wacom_report_touch_mute(wacom, !wacom->shared->is_touch_on);
input_report_abs(input, ABS_RX, strip1);
input_report_abs(input, ABS_RY, strip2);
@@ -2153,7 +2165,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
*/
if ((equivalent_usage == WACOM_HID_WD_MUTE_DEVICE) ||
(equivalent_usage == WACOM_HID_WD_TOUCHONOFF)) {
- if (wacom_wac->shared->touch_input) {
+ if (wacom_wac->shared) {
bool *is_touch_on = &wacom_wac->shared->is_touch_on;
if (equivalent_usage == WACOM_HID_WD_MUTE_DEVICE && value)
@@ -2161,9 +2173,7 @@ static void wacom_wac_pad_event(struct hid_device *hdev, struct hid_field *field
else if (equivalent_usage == WACOM_HID_WD_TOUCHONOFF)
*is_touch_on = value;
- input_report_switch(wacom_wac->shared->touch_input,
- SW_MUTE_DEVICE, !(*is_touch_on));
- input_sync(wacom_wac->shared->touch_input);
+ wacom_report_touch_mute(wacom_wac, !(*is_touch_on));
}
return;
}
@@ -3381,11 +3391,8 @@ static int wacom_wireless_irq(struct wacom_wac *wacom, size_t len)
if ((wacom->shared->type == INTUOSHT ||
wacom->shared->type == INTUOSHT2) &&
- wacom->shared->touch_input &&
wacom->shared->touch_max) {
- input_report_switch(wacom->shared->touch_input,
- SW_MUTE_DEVICE, data[5] & 0x40);
- input_sync(wacom->shared->touch_input);
+ wacom_report_touch_mute(wacom, data[5] & 0x40);
}
pid = get_unaligned_be16(&data[6]);
@@ -3420,11 +3427,8 @@ static int wacom_status_irq(struct wacom_wac *wacom_wac, size_t len)
if ((features->type == INTUOSHT ||
features->type == INTUOSHT2) &&
- wacom_wac->shared->touch_input &&
features->touch_max) {
- input_report_switch(wacom_wac->shared->touch_input,
- SW_MUTE_DEVICE, data[8] & 0x40);
- input_sync(wacom_wac->shared->touch_input);
+ wacom_report_touch_mute(wacom_wac, data[8] & 0x40);
}
if (data[9] & 0x02) { /* wireless module is attached */
@@ -3951,11 +3955,22 @@ int wacom_setup_pen_input_capabilities(struct input_dev *input_dev,
int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
struct wacom_wac *wacom_wac)
{
+ struct wacom *wacom = container_of(wacom_wac, struct wacom, wacom_wac);
+ struct hid_device *hdev = wacom->hdev;
struct wacom_features *features = &wacom_wac->features;
if (!(features->device_type & WACOM_DEVICETYPE_TOUCH))
return -ENODEV;
+ if (features->type != TABLETPC &&
+ features->type != TABLETPC2FG &&
+ features->type != MTSCREEN &&
+ features->type != MTTPC &&
+ features->type != MTTPC_B) {
+ input_dev->evbit[0] |= BIT_MASK(EV_SW);
+ __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+ }
+
if (features->device_type & WACOM_DEVICETYPE_DIRECT)
__set_bit(INPUT_PROP_DIRECT, input_dev->propbit);
else
@@ -3992,22 +4007,17 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
switch (features->type) {
case INTUOSP2_BT:
case INTUOSP2S_BT:
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
-
- if (wacom_wac->shared->touch->product == 0x361) {
+ if (hdev->product == 0x361) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 12440, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
0, 8640, 4, 0);
- }
- else if (wacom_wac->shared->touch->product == 0x360) {
+ } else if (hdev->product == 0x360) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 8960, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
0, 5920, 4, 0);
- }
- else if (wacom_wac->shared->touch->product == 0x393) {
+ } else if (hdev->product == 0x393) {
input_set_abs_params(input_dev, ABS_MT_POSITION_X,
0, 6400, 4, 0);
input_set_abs_params(input_dev, ABS_MT_POSITION_Y,
@@ -4037,10 +4047,8 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
fallthrough;
case WACOM_27QHDT:
- if (wacom_wac->shared->touch->product == 0x32C ||
- wacom_wac->shared->touch->product == 0xF6) {
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
+ if (hdev->product == 0x32C ||
+ hdev->product == 0xF6) {
wacom_wac->has_mute_touch_switch = true;
wacom_wac->is_soft_touch_switch = true;
}
@@ -4059,8 +4067,6 @@ int wacom_setup_touch_input_capabilities(struct input_dev *input_dev,
case INTUOSHT:
case INTUOSHT2:
- input_dev->evbit[0] |= BIT_MASK(EV_SW);
- __set_bit(SW_MUTE_DEVICE, input_dev->swbit);
fallthrough;
case BAMBOO_PT:
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index 126bec6e5c0c..a8bbba4a6f37 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -285,7 +285,7 @@ struct wacom_shared {
/* for wireless device to access USB interfaces */
unsigned touch_max;
int type;
- struct input_dev *touch_input;
+ struct input_dev __rcu *touch_input;
struct hid_device *pen;
struct hid_device *touch;
bool has_mute_touch_switch;
--
2.54.0.1099.g489fc7bff1-goog
^ permalink raw reply related
* [PATCH v3 2/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
From: Lee Jones @ 2026-06-09 12:13 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Dmitry Torokhov, Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <20260609121353.3743782-1-lee@kernel.org>
wacom_bamboo_pad_pen_event() accesses wacom->shared->pen locklessly
relative to wacom_remove_shared_data() which nullifies it. This
can lead to a Use-After-Free if the sibling device is removed while
events are being processed.
Resolve this by introducing RCU protection for pen and touch
pointers:
- Annotate 'pen' and 'touch' in wacom_shared struct with __rcu.
- Wrap lockless readers in wacom_bamboo_pad_pen_event() with
rcu_read_lock() and rcu_dereference().
- Update writers in wacom_sys.c using rcu_assign_pointer().
- Use rcu_dereference_protected for comparisons under
wacom_udev_list_lock.
- Also use rcu_access_pointer in wacom_mode_change_work() to avoid
warnings (while lockless access there remains a pre-existing issue).
Fixes: 8c97a765467c ("HID: wacom: add full support of the Wacom Bamboo PAD")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
drivers/hid/wacom_sys.c | 36 +++++++++++++++++++++++++-----------
drivers/hid/wacom_wac.c | 14 +++++++-------
drivers/hid/wacom_wac.h | 4 ++--
3 files changed, 34 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 7ba589826548..9b352027aa98 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -878,11 +878,18 @@ static void wacom_remove_shared_data(void *res)
shared);
scoped_guard(mutex, &wacom_udev_list_lock) {
- if (wacom_wac->shared->touch == wacom->hdev) {
- wacom_wac->shared->touch = NULL;
+ struct hid_device *touch =
+ rcu_dereference_protected(wacom_wac->shared->touch,
+ lockdep_is_held(&wacom_udev_list_lock));
+ struct hid_device *pen =
+ rcu_dereference_protected(wacom_wac->shared->pen,
+ lockdep_is_held(&wacom_udev_list_lock));
+
+ if (touch == wacom->hdev) {
+ rcu_assign_pointer(wacom_wac->shared->touch, NULL);
rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
- } else if (wacom_wac->shared->pen == wacom->hdev) {
- wacom_wac->shared->pen = NULL;
+ } else if (pen == wacom->hdev) {
+ rcu_assign_pointer(wacom_wac->shared->pen, NULL);
}
}
@@ -916,9 +923,9 @@ static int wacom_add_shared_data(struct hid_device *hdev)
}
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
- data->shared.touch = hdev;
+ rcu_assign_pointer(data->shared.touch, hdev);
else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
- data->shared.pen = hdev;
+ rcu_assign_pointer(data->shared.pen, hdev);
mutex_unlock(&wacom_udev_list_lock);
@@ -2356,7 +2363,11 @@ static void wacom_set_shared_values(struct wacom_wac *wacom_wac)
mutex_lock(&wacom_udev_list_lock);
if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH) {
- if (wacom_wac->shared->touch == wacom->hdev) {
+ struct hid_device *touch =
+ rcu_dereference_protected(wacom_wac->shared->touch,
+ lockdep_is_held(&wacom_udev_list_lock));
+
+ if (touch == wacom->hdev) {
wacom_wac->shared->type = wacom_wac->features.type;
rcu_assign_pointer(wacom_wac->shared->touch_input, wacom_wac->touch_input);
}
@@ -2797,16 +2808,19 @@ static void wacom_mode_change_work(struct work_struct *work)
bool is_direct = wacom->wacom_wac.is_direct_mode;
int error = 0;
- if (shared->pen) {
- wacom1 = hid_get_drvdata(shared->pen);
+ struct hid_device *pen = rcu_access_pointer(shared->pen);
+ struct hid_device *touch = rcu_access_pointer(shared->touch);
+
+ if (pen) {
+ wacom1 = hid_get_drvdata(pen);
wacom_release_resources(wacom1);
hid_hw_stop(wacom1->hdev);
wacom1->wacom_wac.has_mode_change = true;
wacom1->wacom_wac.is_direct_mode = is_direct;
}
- if (shared->touch) {
- wacom2 = hid_get_drvdata(shared->touch);
+ if (touch) {
+ wacom2 = hid_get_drvdata(touch);
wacom_release_resources(wacom2);
hid_hw_stop(wacom2->hdev);
wacom2->wacom_wac.has_mode_change = true;
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
index 495960227b8d..32d6f1dfb001 100644
--- a/drivers/hid/wacom_wac.c
+++ b/drivers/hid/wacom_wac.c
@@ -3296,6 +3296,7 @@ static int wacom_bpt_irq(struct wacom_wac *wacom, size_t len)
static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
unsigned char *data)
{
+ struct hid_device *pen;
unsigned char prefix;
/*
@@ -3308,13 +3309,12 @@ static void wacom_bamboo_pad_pen_event(struct wacom_wac *wacom,
prefix = data[0];
data[0] = WACOM_REPORT_BPAD_PEN;
- /*
- * actually reroute the event.
- * No need to check if wacom->shared->pen is valid, hid_input_report()
- * will check for us.
- */
- hid_input_report(wacom->shared->pen, HID_INPUT_REPORT, data,
- WACOM_PKGLEN_PENABLED, 1);
+ rcu_read_lock();
+ pen = rcu_dereference(wacom->shared->pen);
+ if (pen)
+ hid_input_report(pen, HID_INPUT_REPORT, data,
+ WACOM_PKGLEN_PENABLED, 1);
+ rcu_read_unlock();
data[0] = prefix;
}
diff --git a/drivers/hid/wacom_wac.h b/drivers/hid/wacom_wac.h
index a8bbba4a6f37..170d6adbe02a 100644
--- a/drivers/hid/wacom_wac.h
+++ b/drivers/hid/wacom_wac.h
@@ -286,8 +286,8 @@ struct wacom_shared {
unsigned touch_max;
int type;
struct input_dev __rcu *touch_input;
- struct hid_device *pen;
- struct hid_device *touch;
+ struct hid_device __rcu *pen;
+ struct hid_device __rcu *touch;
bool has_mute_touch_switch;
bool is_touch_on;
};
--
2.54.0.1099.g489fc7bff1-goog
^ permalink raw reply related
* [PATCH v3 3/4] HID: wacom: Redesign shared sibling data lifecycle
From: Lee Jones @ 2026-06-09 12:13 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20260609121353.3743782-1-lee@kernel.org>
The Wacom driver coordinates state between sibling interfaces of
the same physical device using a shared structure 'wacom_shared'
inside 'wacom_hdev_data'. The driver kept a volatile representative
pointer 'data->dev' pointing to a sibling 'hid_device' for physical
path comparisons during sibling matching.
This pointer management is fragile. When the representative device
is disconnected, wacom_remove_shared_data() failed to clear/update
'data->dev', leading to a Use-After-Free vulnerability when
subsequent sibling probes dereference the dangling 'data->dev'
pointer.
Resolve this issue by redesigning the sibling data lifecycle:
- Eliminate the volatile 'data->dev' representative pointer
completely
- Redesign 'wacom_hdev_data' to store stable static copies of the
required attributes upon first allocation: 'phys' path string,
'vendor', 'product' IDs and the sibling's 'device_type'
- Use these static attributes for stable sibling matching in
wacom_are_sibling() and wacom_get_hdev_data()
This ensures sibling matching remains safe and stable even if
individual siblings are dynamically added or removed.
To secure the lifecycle against concurrent probe/disconnect races:
- Switch kref_put() to kref_put_mutex() in
wacom_remove_shared_data() to serialize refcount drops with list
traversal and lookup
- Modify wacom_release_shared_data() to assume the list lock is
already held
Also, do not accumulate the 'device_type' capability flag during
subsequent sibling probes. Keeping only the first probed sibling's
device_type exactly preserves the original sibling matching behavior
without introducing side effects.
Fixes: 4492efffffeb ("Input: wacom - share pen info with touch of the same ID")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
drivers/hid/wacom_sys.c | 70 +++++++++++++++++++++++++++--------------
1 file changed, 46 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 9b352027aa98..1b019e3331b4 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -759,27 +759,47 @@ static void wacom_retrieve_hid_descriptor(struct hid_device *hdev,
struct wacom_hdev_data {
struct list_head list;
struct kref kref;
- struct hid_device *dev;
+ char phys[64];
+ __u32 vendor;
+ __u32 product;
+ __u32 device_type;
struct wacom_shared shared;
};
+static bool wacom_compare_device_paths(struct hid_device *hdev_a,
+ const char *phys_b, char separator)
+{
+ const char *p1 = strrchr(hdev_a->phys, separator);
+ const char *p2 = strrchr(phys_b, separator);
+ int n1, n2;
+
+ if (!p1 || !p2)
+ return false;
+
+ n1 = p1 - hdev_a->phys;
+ n2 = p2 - phys_b;
+
+ if (n1 != n2 || n1 <= 0 || n2 <= 0)
+ return false;
+
+ return !strncmp(hdev_a->phys, phys_b, n1);
+}
+
static LIST_HEAD(wacom_udev_list);
static DEFINE_MUTEX(wacom_udev_list_lock);
static bool wacom_are_sibling(struct hid_device *hdev,
- struct hid_device *sibling)
+ struct wacom_hdev_data *data)
{
struct wacom *wacom = hid_get_drvdata(hdev);
struct wacom_features *features = &wacom->wacom_wac.features;
- struct wacom *sibling_wacom = hid_get_drvdata(sibling);
- struct wacom_features *sibling_features = &sibling_wacom->wacom_wac.features;
__u32 oVid = features->oVid ? features->oVid : hdev->vendor;
__u32 oPid = features->oPid ? features->oPid : hdev->product;
/* The defined oVid/oPid must match that of the sibling */
- if (features->oVid != HID_ANY_ID && sibling->vendor != oVid)
+ if (features->oVid != HID_ANY_ID && data->vendor != oVid)
return false;
- if (features->oPid != HID_ANY_ID && sibling->product != oPid)
+ if (features->oPid != HID_ANY_ID && data->product != oPid)
return false;
/*
@@ -787,11 +807,11 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* device path, while those with different VID/PID must share
* the same physical parent device path.
*/
- if (hdev->vendor == sibling->vendor && hdev->product == sibling->product) {
- if (!hid_compare_device_paths(hdev, sibling, '/'))
+ if (hdev->vendor == data->vendor && hdev->product == data->product) {
+ if (!wacom_compare_device_paths(hdev, data->phys, '/'))
return false;
} else {
- if (!hid_compare_device_paths(hdev, sibling, '.'))
+ if (!wacom_compare_device_paths(hdev, data->phys, '.'))
return false;
}
@@ -804,7 +824,7 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* devices.
*/
if ((features->device_type & WACOM_DEVICETYPE_DIRECT) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+ !(data->device_type & WACOM_DEVICETYPE_DIRECT))
return false;
/*
@@ -812,17 +832,17 @@ static bool wacom_are_sibling(struct hid_device *hdev,
* devices.
*/
if (!(features->device_type & WACOM_DEVICETYPE_DIRECT) &&
- (sibling_features->device_type & WACOM_DEVICETYPE_DIRECT))
+ (data->device_type & WACOM_DEVICETYPE_DIRECT))
return false;
/* Pen devices may only be siblings of touch devices */
if ((features->device_type & WACOM_DEVICETYPE_PEN) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_TOUCH))
+ !(data->device_type & WACOM_DEVICETYPE_TOUCH))
return false;
/* Touch devices may only be siblings of pen devices */
if ((features->device_type & WACOM_DEVICETYPE_TOUCH) &&
- !(sibling_features->device_type & WACOM_DEVICETYPE_PEN))
+ !(data->device_type & WACOM_DEVICETYPE_PEN))
return false;
/*
@@ -838,7 +858,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
/* Try to find an already-probed interface from the same device */
list_for_each_entry(data, &wacom_udev_list, list) {
- if (hid_compare_device_paths(hdev, data->dev, '/')) {
+ if (wacom_compare_device_paths(hdev, data->phys, '/')) {
kref_get(&data->kref);
return data;
}
@@ -846,7 +866,7 @@ static struct wacom_hdev_data *wacom_get_hdev_data(struct hid_device *hdev)
/* Fallback to finding devices that appear to be "siblings" */
list_for_each_entry(data, &wacom_udev_list, list) {
- if (wacom_are_sibling(hdev, data->dev)) {
+ if (wacom_are_sibling(hdev, data)) {
kref_get(&data->kref);
return data;
}
@@ -860,18 +880,15 @@ static void wacom_release_shared_data(struct kref *kref)
struct wacom_hdev_data *data =
container_of(kref, struct wacom_hdev_data, kref);
- mutex_lock(&wacom_udev_list_lock);
list_del(&data->list);
- mutex_unlock(&wacom_udev_list_lock);
-
kfree(data);
}
static void wacom_remove_shared_data(void *res)
{
- struct wacom *wacom = res;
+ struct wacom *res_wacom = res;
struct wacom_hdev_data *data;
- struct wacom_wac *wacom_wac = &wacom->wacom_wac;
+ struct wacom_wac *wacom_wac = &res_wacom->wacom_wac;
if (wacom_wac->shared) {
data = container_of(wacom_wac->shared, struct wacom_hdev_data,
@@ -885,17 +902,19 @@ static void wacom_remove_shared_data(void *res)
rcu_dereference_protected(wacom_wac->shared->pen,
lockdep_is_held(&wacom_udev_list_lock));
- if (touch == wacom->hdev) {
+ if (touch == res_wacom->hdev) {
rcu_assign_pointer(wacom_wac->shared->touch, NULL);
rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
- } else if (pen == wacom->hdev) {
+ } else if (pen == res_wacom->hdev) {
rcu_assign_pointer(wacom_wac->shared->pen, NULL);
}
}
synchronize_rcu();
- kref_put(&data->kref, wacom_release_shared_data);
+ if (kref_put_mutex(&data->kref, wacom_release_shared_data, &wacom_udev_list_lock))
+ mutex_unlock(&wacom_udev_list_lock);
+
wacom_wac->shared = NULL;
}
}
@@ -918,7 +937,10 @@ static int wacom_add_shared_data(struct hid_device *hdev)
}
kref_init(&data->kref);
- data->dev = hdev;
+ strscpy(data->phys, hdev->phys, sizeof(data->phys));
+ data->vendor = hdev->vendor;
+ data->product = hdev->product;
+ data->device_type = wacom_wac->features.device_type;
list_add_tail(&data->list, &wacom_udev_list);
}
--
2.54.0.1099.g489fc7bff1-goog
^ permalink raw reply related
* [PATCH v3 4/4] HID: wacom: Fix teardown order in wacom_mode_change_work
From: Lee Jones @ 2026-06-09 12:13 UTC (permalink / raw)
To: lee, Ping Cheng, Jason Gerecke, Jiri Kosina, Benjamin Tissoires,
Peter Hutterer, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20260609121353.3743782-1-lee@kernel.org>
wacom_mode_change_work() called wacom_release_resources() before
hid_hw_stop(). wacom_release_resources() triggers devres cleanup
which calls wacom_remove_shared_data() to nullify wacom_wac->shared.
Since hid_hw_stop() was not called yet, hardware IRQs could still fire
and dereference the nullified wacom_wac->shared pointer, causing a crash.
Fix this by correcting the teardown order to call hid_hw_stop()
first, matching the order used in wacom_remove().
Fixes: 4082da80f46a ("HID: wacom: generic: add mode change touch key")
Signed-off-by: Lee Jones <lee@kernel.org>
---
v1 -> v2: Split and use RCU as per Dmitry's review
v2 -> v3: Sashiko fixes
drivers/hid/wacom_sys.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 1b019e3331b4..378dddbf7ec5 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2835,16 +2835,16 @@ static void wacom_mode_change_work(struct work_struct *work)
if (pen) {
wacom1 = hid_get_drvdata(pen);
- wacom_release_resources(wacom1);
hid_hw_stop(wacom1->hdev);
+ wacom_release_resources(wacom1);
wacom1->wacom_wac.has_mode_change = true;
wacom1->wacom_wac.is_direct_mode = is_direct;
}
if (touch) {
wacom2 = hid_get_drvdata(touch);
- wacom_release_resources(wacom2);
hid_hw_stop(wacom2->hdev);
+ wacom_release_resources(wacom2);
wacom2->wacom_wac.has_mode_change = true;
wacom2->wacom_wac.is_direct_mode = is_direct;
}
--
2.54.0.1099.g489fc7bff1-goog
^ permalink raw reply related
* Re: [PATCH v3 2/4] HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
From: sashiko-bot @ 2026-06-09 12:33 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-input, dmitry.torokhov
In-Reply-To: <20260609121353.3743782-2-lee@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `wacom_mode_change_work()` misuses `rcu_access_pointer()` to fetch RCU-protected pointers without holding `rcu_read_lock()`, leading to a Use-After-Free.
--
commit 4eb3f98aa9f594548c94852bcf57eb40ae946cd8
Author: Lee Jones <lee@kernel.org>
Subject: HID: wacom: Fix Use-After-Free in wacom_bamboo_pad
This commit resolves a Use-After-Free vulnerability in
wacom_bamboo_pad_pen_event() by introducing RCU protection for the pen and
touch pointers within the wacom_shared structure. It annotates the pointers
with __rcu, wraps lockless read access with rcu_read_lock() and
rcu_dereference(), and updates writers using the appropriate RCU assignment
primitives. It also updates pointer access in wacom_mode_change_work() to use
rcu_access_pointer() to avoid warnings.
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 7ba5898265481..9b352027aa980 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2797,16 +2808,19 @@ static void wacom_mode_change_work(struct work_struct *work)
> bool is_direct = wacom->wacom_wac.is_direct_mode;
> int error = 0;
>
> - if (shared->pen) {
> - wacom1 = hid_get_drvdata(shared->pen);
> + struct hid_device *pen = rcu_access_pointer(shared->pen);
> + struct hid_device *touch = rcu_access_pointer(shared->touch);
> +
> + if (pen) {
> + wacom1 = hid_get_drvdata(pen);
> wacom_release_resources(wacom1);
> hid_hw_stop(wacom1->hdev);
[Severity: High]
This isn't a bug introduced by this patch, but does wacom_mode_change_work()
misuse rcu_access_pointer() here by fetching RCU-protected pointers without
holding rcu_read_lock(), which still leaves an exploitable Use-After-Free?
If the sibling device is unbound concurrently, its wacom_remove() triggers
wacom_remove_shared_data(), which nullifies the shared pointer and calls
synchronize_rcu(). Because the worker does not hold rcu_read_lock(),
synchronize_rcu() will not wait for it.
Doesn't this allow the sibling device to be completely freed while the worker
is still actively dereferencing its memory via hid_get_drvdata(),
wacom_release_resources(), and hid_hw_stop()?
Could using rcu_access_pointer() (which is explicitly intended for pointers
that will not be dereferenced) be papering over the underlying race condition
just to silence sparse/lockdep warnings?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121353.3743782-1-lee@kernel.org?part=2
^ permalink raw reply
* Re: [PATCH v3 1/4] HID: wacom: Fix Use-After-Free in wacom_intuos_pad
From: sashiko-bot @ 2026-06-09 12:35 UTC (permalink / raw)
To: Lee Jones; +Cc: dmitry.torokhov, linux-input
In-Reply-To: <20260609121353.3743782-1-lee@kernel.org>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Lockless TOCTOU Use-After-Free of sibling devices in wacom_mode_change_work
--
commit bb8e808fb26bf4a0ca1a10604a506fb1d60c8173
Author: Lee Jones <lee@kernel.org>
HID: wacom: Fix Use-After-Free in wacom_intuos_pad
This commit resolves a time-of-check to time-of-use race condition in
wacom_intuos_pad() that could lead to a use-after-free. It introduces
RCU protection for the touch_input pointer, wrapping lockless readers
with rcu_read_lock() and writers with rcu_assign_pointer().
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..7ba5898265481 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -877,10 +877,16 @@ static void wacom_remove_shared_data(void *res)
> data = container_of(wacom_wac->shared, struct wacom_hdev_data,
> shared);
>
> - if (wacom_wac->shared->touch == wacom->hdev)
> - wacom_wac->shared->touch = NULL;
> - else if (wacom_wac->shared->pen == wacom->hdev)
> - wacom_wac->shared->pen = NULL;
> + scoped_guard(mutex, &wacom_udev_list_lock) {
> + if (wacom_wac->shared->touch == wacom->hdev) {
> + wacom_wac->shared->touch = NULL;
> + rcu_assign_pointer(wacom_wac->shared->touch_input, NULL);
> + } else if (wacom_wac->shared->pen == wacom->hdev) {
> + wacom_wac->shared->pen = NULL;
> + }
> + }
> +
> + synchronize_rcu();
[Severity: High]
This is a pre-existing issue, but is it possible for a sibling device to
still be used after this synchronize_rcu() call?
Looking at wacom_mode_change_work(), it reads shared->pen and shared->touch
locklessly:
drivers/hid/wacom_sys.c:wacom_mode_change_work() {
...
if (shared->pen) {
wacom1 = hid_get_drvdata(shared->pen);
wacom_release_resources(wacom1);
hid_hw_stop(wacom1->hdev);
...
}
If a context switch occurs right after shared->pen is evaluated as true,
could a sibling device be physically disconnected, triggering
wacom_remove_shared_data() to nullify shared->pen and free the object?
Because wacom_mode_change_work() does not hold an rcu_read_lock(), this
synchronize_rcu() wouldn't block it. When the workqueue resumes, could it
pass the now-freed pointer to hid_get_drvdata() and cause a use-after-free?
>
> kref_put(&data->kref, wacom_release_shared_data);
> wacom_wac->shared = NULL;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260609121353.3743782-1-lee@kernel.org?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