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

[sashiko-bot@kernel.org]

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