[PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow

[PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow

[Tianchu Chen]

From: Tianchu Chen <flynnnchen@tencent.com>

goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
buffer (tmp_buf), writing an 11-12 byte header followed by the
caller-supplied report data.  The HID core caps report size at
HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
hid_ll_driver.max_buffer_size and performs no bounds checking before
copying the payload:

    memcpy(tmp_buf + tx_len, buf, len);

A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
overflows the stack buffer.

Add a size check after constructing the header, rejecting reports that
would exceed the buffer capacity.

Discovered by Atuin - Automated Vulnerability Discovery Engine.

Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
Cc: stable@vger.kernel.org
Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
---
 drivers/hid/hid-goodix-spi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
index 80c0288a3..288cb827e 100644
--- a/drivers/hid/hid-goodix-spi.c
+++ b/drivers/hid/hid-goodix-spi.c
@@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
 	memcpy(tmp_buf + tx_len, args, args_len);
 	tx_len += args_len;
 
+	if (tx_len + len > sizeof(tmp_buf))
+		return -EINVAL;
+
 	memcpy(tmp_buf + tx_len, buf, len);
 	tx_len += len;
 
-- 
2.51.0

Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow

[sashiko-bot]

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

New issues:
- [Medium] The patch artificially caps SET_REPORT payload size to a 128-byte stack buffer, potentially breaking legitimate large HID reports.

Pre-existing issues:
- [High] Concurrent reallocation of `ts->event_buf` in `goodix_hid_start` and access in `goodix_hid_irq` leads to a Use-After-Free.
- [Medium] Uninitialized kernel memory is transmitted over the SPI bus during read operations.
- [Medium] The driver performs SPI reads and writes without verifying the hardware suspend state.
- [Medium] In `goodix_spi_resume()`, the IRQ is enabled before the device is powered on.
- [Medium] Unsafe assignment of `devm_krealloc` directly to the buffer pointer causes a device-lifetime memory leak.
--

commit a3440b67c988b5226da753835060ad3d93591c74
Author: Tianchu Chen <flynnnchen@tencent.com>
Subject: HID: hid-goodix-spi: validate report size to prevent stack buffer overflow

This commit adds a bounds check in goodix_hid_set_raw_report() to ensure that
the constructed protocol frame does not exceed the size of the 128-byte stack
buffer tmp_buf. If a caller supplies a report that exceeds this limit, the
function now returns -EINVAL to prevent a stack buffer overflow.

> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3a38b..288cb827e9d6d 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
>  	memcpy(tmp_buf + tx_len, args, args_len);
>  	tx_len += args_len;
>  
> +	if (tx_len + len > sizeof(tmp_buf))
> +		return -EINVAL;

[Severity: Medium]
Does returning `-EINVAL` here artificially cap the SET_REPORT payload size?
If the device's HID report descriptor defines larger output or feature reports
(e.g., for firmware updates or configuration), would this fix block them?
Could we dynamically allocate a buffer of adequate size or use the already
available 2KB `ts->xfer_buf` instead of relying on a hardcoded stack limit?

> +
>  	memcpy(tmp_buf + tx_len, buf, len);
>  	tx_len += len;
>  

[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk with
`ts->event_buf` between `goodix_hid_irq()` and `goodix_hid_start()`?

If the HID driver is unbound and rebound, `goodix_hid_start()` will execute
`devm_krealloc()`, freeing the old `ts->event_buf`:

drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
    ...
	ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
                                  report_size, GFP_KERNEL);
    ...
}

Meanwhile, if `goodix_hid_close()` clears the `GOODIX_HID_STARTED` flag but
does not synchronize with the threaded IRQ, a preempted IRQ handler can pass
the flag check and then use the now-freed `ts->event_buf`:

drivers/hid/hid-goodix-spi.c:goodix_hid_irq() {
    ...
	if (!test_bit(GOODIX_HID_STARTED, &ts->flags))
		return IRQ_HANDLED;
    ...
	event = goodix_get_event_report(ts, ts->hid_report_addr, ts->event_buf,
					GOODIX_HID_ACK_HEADER_SIZE +
					GOODIX_HID_COOR_PKG_LEN);
    ...
}


[Severity: Medium]
This is a pre-existing issue, but does assigning the result of
`devm_krealloc()` directly to `ts->event_buf` cause a memory leak if it fails?

drivers/hid/hid-goodix-spi.c:goodix_hid_start() {
    ...
	ts->event_buf = devm_krealloc(ts->dev, ts->event_buf,
                                  report_size, GFP_KERNEL);
    ...
}

If `devm_krealloc()` returns `NULL`, the original memory chunk remains in the
`devres` list, but the pointer in `ts->event_buf` is overwritten. Subsequent
re-binds would allocate a new devres chunk instead of reusing the old one,
accumulating leaked chunks until the SPI device is unbound.


[Severity: Medium]
This is a pre-existing issue, but does this transmit uninitialized kernel
memory over the SPI MOSI line during read operations?

In `goodix_get_event_report()`, `ts->event_buf` is used as the transaction
buffer, and it is allocated with `devm_kmalloc()` (which doesn't zero memory).
The first 5 bytes are initialized, but the dummy bytes and read payload
section are left uninitialized:

drivers/hid/hid-goodix-spi.c:goodix_get_event_report() {
    ...
	data[0] = GOODIX_SPI_READ_FLAG;
	put_unaligned_be32(addr, data + GOODIX_SPI_TRANS_PREFIX_LEN);

	spi_message_init(&spi_msg);
	memset(&xfers, 0, sizeof(xfers));
	xfers.tx_buf = data;
	xfers.rx_buf = data;
	xfers.len = GOODIX_SPI_READ_PREFIX_LEN + len;
    ...
}

Because `xfers.tx_buf` is provided for the entire transfer length, is the
uninitialized memory sent to the hardware during the dummy and read phases,
resulting in an information leak?


[Severity: Medium]
This is a pre-existing issue, but does `goodix_hid_raw_request()` perform SPI
operations without verifying the hardware suspend state?

drivers/hid/hid-goodix-spi.c:goodix_hid_raw_request() {
    ...
	guard(mutex)(&ts->hid_request_lock);
	switch (reqtype) {
	case HID_REQ_GET_REPORT:
		error = goodix_hid_get_raw_report(hid, reportnum, buf,
						  len, rtype);
		break;
	case HID_REQ_SET_REPORT:
		if (buf[0] == reportnum)
			error = goodix_hid_set_raw_report(hid, reportnum,
							  buf, len, rtype);
		break;
    ...
}

If a request is issued while the device is in the `GOODIX_SPI_POWER_SLEEP`
state, the driver will directly perform SPI read/write operations on the
powered-down hardware. Could this cause timeouts or undefined hardware states?


[Severity: Medium]
This is a pre-existing issue, but should the IRQ be enabled before the device
is powered on during resume?

drivers/hid/hid-goodix-spi.c:goodix_spi_resume() {
    ...
	enable_irq(ts->spi->irq);
	return goodix_spi_set_power(ts, GOODIX_SPI_POWER_ON);
}

If a spurious interrupt or power-on glitch asserts the IRQ line while the
device is still in the sleep state, the interrupt handler will execute and
attempt to perform SPI reads on the waking hardware. Would it be safer to
bring the device to an active power state before unmasking its interrupts?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/f7e444a3facbe5fb2627167ab205771476e46bc8@linux.dev?part=1

Re: [PATCH] HID: hid-goodix-spi: validate report size to prevent stack buffer overflow

[Dmitry Torokhov]

On Fri, May 29, 2026 at 01:42:47PM +0000, Tianchu Chen wrote:
> From: Tianchu Chen <flynnnchen@tencent.com>
> 
> goodix_hid_set_raw_report() builds a protocol frame in a 128-byte stack
> buffer (tmp_buf), writing an 11-12 byte header followed by the
> caller-supplied report data.  The HID core caps report size at
> HID_MAX_BUFFER_SIZE (16384) by default, while the driver does not set
> hid_ll_driver.max_buffer_size and performs no bounds checking before
> copying the payload:
> 
>     memcpy(tmp_buf + tx_len, buf, len);
> 
> A hidraw SET_REPORT ioctl with a report larger than ~116 bytes
> overflows the stack buffer.
> 
> Add a size check after constructing the header, rejecting reports that
> would exceed the buffer capacity.
> 
> Discovered by Atuin - Automated Vulnerability Discovery Engine.
> 
> Fixes: 75e16c8ce283 ("HID: hid-goodix: Add Goodix HID-over-SPI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Tianchu Chen <flynnnchen@tencent.com>
> ---
>  drivers/hid/hid-goodix-spi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/hid-goodix-spi.c b/drivers/hid/hid-goodix-spi.c
> index 80c0288a3..288cb827e 100644
> --- a/drivers/hid/hid-goodix-spi.c
> +++ b/drivers/hid/hid-goodix-spi.c
> @@ -520,6 +520,9 @@ static int goodix_hid_set_raw_report(struct hid_device *hid,
>  	memcpy(tmp_buf + tx_len, args, args_len);
>  	tx_len += args_len;
>  
> +	if (tx_len + len > sizeof(tmp_buf))
> +		return -EINVAL;
> +

We can also consider returning -E2BIG here.

>  	memcpy(tmp_buf + tx_len, buf, len);
>  	tx_len += len;
>  

In any case:

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

I think we can ignore Sashiko's ramblings on this patch, it needs some
instructions detailing order of operations/timing of callbacks in HID
subsystem.

Thanks.

-- 
Dmitry