Re: [PATCH v4 06/11] HID: spi_hid: add spi_hid traces

[sashiko-bot@kernel.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