Re: [PATCH] HID: intel-thc-hid: reject oversized QuickSPI GET_REPORT responses

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

Pre-existing issues:
- [High] Missing bounds check on user-supplied buffer length in `quickspi_set_report` leads to a heap buffer overflow in `write_cmd_to_txdma`.
- [High] Preexisting heap buffer overflows in the QuickSPI DMA input reporting paths due to 4KB-aligned boundary checks and unaligned allocations.
- [High] Concurrent `raw_request` calls are not serialized, leading to data races and corruption on the shared `qsdev->report_buf`.
- [High] Incomplete fix in `quickspi_get_report` protects the user buffer but fails to prevent an out-of-bounds read from the kernel heap.
--

--- Patch [1]: [PATCH] HID: intel-thc-hid: reject oversized QuickSPI GET_REPORT responses ---
commit 95a4cfcd8aa1ba2ab8a0e552fbf091c155176036
Author: Jérémy Jean <[email protected]>

HID: intel-thc-hid: reject oversized QuickSPI GET_REPORT responses

Thread the HID core caller length into quickspi_get_report() and reject
responses that exceed the caller-provided buffer before the final copy.
Snapshot the completed response length once so the bound check, copy, and
returned byte count all use the same value.

Fixes: 9d8d51735a3a ("HID: intel-thc-hid: intel-quickspi: Add HIDSPI protocol implementation")
Assisted-by: Codex:gpt-5
Signed-off-by: Jérémy Jean <[email protected]>

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
> index 91d5807b4a830..a60a0a7f16aad 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c
> @@ -61,7 +61,7 @@ static int quickspi_hid_raw_request(struct hid_device *hid,
>  
>  	switch (reqtype) {
>  	case HID_REQ_GET_REPORT:
> -		ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
> +		ret = quickspi_get_report(qsdev, rtype, reportnum, buf, len);

[Severity: High]
This isn't a bug introduced by this patch, but are concurrent raw_request
calls serialized here?

It looks like multiple threads could invoke quickspi_hid_raw_request()
simultaneously from userspace. Without a driver-level mutex, could
concurrent threads write outgoing commands directly into the single shared
qsdev->report_buf in write_cmd_to_txdma()?

>  		break;
>  	case HID_REQ_SET_REPORT:
>  		ret = quickspi_set_report(qsdev, rtype, reportnum, buf, len);

[Severity: High]
This is a pre-existing issue, but does this pass an unvalidated user length
through to quickspi_set_report()?

If userspace issues a SET_REPORT request with a payload up to 4096 bytes, len
is passed to write_cmd_to_txdma() without being validated against the
allocation size of the internal buffer. Could this unconditionally execute:

memcpy(write_buf->content, report_buf, report_buf_len);

into qsdev->report_buf and overflow the kernel heap?

>  		break;
>  	default:

[ ... ]

> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> index cb19057f1191b..acc9d67c53ca0 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c

[ ... ]

> @@ -371,10 +373,17 @@ int quickspi_get_report(struct quickspi_device *qsdev,
>  		return -ETIMEDOUT;
>  	}
>  	qsdev->get_report_cmpl = false;
> +	report_len = READ_ONCE(qsdev->report_len);
> +
> +	if (report_len > buf_len) {
> +		dev_err_once(qsdev->dev, "Get report response too large: %u vs %zu\n",
> +			     report_len, buf_len);
> +		return -EINVAL;
> +	}
>  
> -	memcpy(buf, qsdev->report_buf, qsdev->report_len);
> +	memcpy(buf, qsdev->report_buf, report_len);

[Severity: High]
This is a pre-existing issue, but does this check prevent out-of-bounds reads
from qsdev->report_buf?

While the new bounds check prevents overflowing the destination buf, it
doesn't seem to verify if report_len exceeds the allocation size of the source
buffer qsdev->report_buf.

Since report_len is derived from an incoming hardware packet, if userspace
requests a large buffer length, could this memcpy read past the end of
qsdev->report_buf and leak adjacent kernel heap memory to userspace?

[Severity: High]
This isn't a bug introduced by this patch, but how is the capacity of
qsdev->report_buf validated against hardware DMA limits during allocation and
reporting?

It appears qsdev->input_buf and qsdev->report_buf are allocated to a precise
descriptor size in quickspi_alloc_report_buf(). However, the DMA limits are
aligned to 4KB in read_dma_buffer():

if (mes_len > read_config->max_packet_size) { ... }
ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);

If a device sends a packet larger than the descriptor size but smaller than
the 4KB-aligned max packet size, could this unconditionally copy the payload
into qsdev->report_buf and overflow the heap?

> -	return qsdev->report_len;
> +	return report_len;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1