Re: [PATCH 1/2] HID: intel-thc-hid: intel-quickspi: size the input buffer for the DMA

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Heap buffer overflow in intel-quicki2c when receiving oversized DMA packets.
- [High] Out-of-bounds kernel memory read in intel-quicki2c leading to an information leak.
- [High] Use-After-Free of DMA buffers during driver teardown due to missing IRQ synchronization.
- [Medium] Transport drivers violate HID subsystem guidelines by using the unsafe hid_input_report API.
--

--- Patch [1]: [PATCH 1/2] HID: intel-thc-hid: intel-quickspi: size the input buffer for the DMA ---
commit b291dd6cc5d97a1fbb9fb9824d1d976e0d317c24
Author: HyeongJun An <sammiee5311@gmail.com>

HID: intel-thc-hid: intel-quickspi: size the input buffer for the DMA

quickspi_alloc_report_buf() sizes input_buf from max_input_len, but the
RXDMA2 channel it feeds is programmed with ALIGN(max_input_len, SZ_4K):
quickspi_dma_init() passes the raw value and dma_set_max_packet_size()
rounds it up.  setup_dma_buffers() maps that rounded size and
read_dma_buffer() bounds the message against it, so a controller
reporting max_input_len 64 and rep_desc_len 600 gets a 600 byte buffer
behind a 4096 byte DMA.  thc_rxdma_read() has no capacity argument, so
nothing downstream can catch it.

> The intel-quicki2c sibling floors its input buffer at SZ_4K for this
> reason, with the comment "give default 4K buffer to avoid DMA buffer
> overrun".  Round up the same way the DMA does.

[Severity: High]
This isn't a bug introduced by this patch, but does intel-quicki2c actually
suffer from the same DMA buffer overrun you are fixing here?

Looking at `quicki2c_alloc_report_buf()` in `pci-quicki2c.c`, `input_buf` is
allocated using `max(..., SZ_4K)` instead of `ALIGN(..., SZ_4K)`:

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:quicki2c_alloc_report_buf() {
	max_report_len = max(le16_to_cpu(qcdev->dev_desc.max_input_len), SZ_4K);
	qcdev->input_buf = devm_kzalloc(qcdev->dev, max_report_len, GFP_KERNEL);
}

If a device declares a `max_input_len` of 5000 bytes, `input_buf` will be sized
to exactly 5000 bytes. However, `thc_dma_set_max_packet_sizes()` unconditionally
rounds the DMA RX size up to the next 4K boundary (8192 bytes).

When an oversized packet arrives, `thc_rxdma_read()` validates it against the
8192-byte limit and copies the excess data into the smaller 5000-byte
`input_buf`, which seems to overflow it:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:read_dma_buffer() {
	if (mes_len > read_config->max_packet_size) { ... }
	...
	ret = sg_copy_to_buffer(sg, nent, read_buff, mes_len);
}

Could `intel-quicki2c` be updated to use `ALIGN()` as well?


[Severity: High]
This is a pre-existing issue, but can a malicious device cause an out-of-bounds
read by sending a short physical packet with a maliciously large length header?

In `handle_input_report()` of `pci-quicki2c.c`, the driver reads an interrupt
payload via `thc_rxdma_read()`, which populates `report_len` with the actual
number of bytes transferred. However, the driver directly uses the
device-provided length field `pkt->len` from the packet header:

drivers/hid/intel-thc-hid/intel-quicki2c/pci-quicki2c.c:handle_input_report() {
	quicki2c_hid_send_report(qcdev, pkt->data, HIDI2C_DATA_LEN(le16_to_cpu(pkt->len)));
}

If `pkt->len` exceeds `report_len`, does the HID core read past the valid packet
data bounds into adjacent kernel heap memory?


> Fixes: 4138f21115ae ("HID: intel-thc-hid: intel-quickspi: Complete THC QuickSPI driver")
> Cc: stable@vger.kernel.org
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
> 
> ---
> drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> index 89226f5ce45e0..9ef33574c202b 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c

[ ... ]

> @@ -549,8 +550,12 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
>  	if (!qsdev->report_descriptor)
>  		return -ENOMEM;
>  
> -	max_input_len = max(le16_to_cpu(qsdev->dev_desc.rep_desc_len),
> -			    le16_to_cpu(qsdev->dev_desc.max_input_len));
> +	/*
> +	 * thc_dma_set_max_packet_sizes() rounds the RXDMA2 packet size up to
> +	 * 4K, so the DMA can hand back more than max_input_len bytes.
> +	 */
> +	max_input_len = max_t(size_t, le16_to_cpu(qsdev->dev_desc.rep_desc_len),
> +			      ALIGN(le16_to_cpu(qsdev->dev_desc.max_input_len), SZ_4K));
>  
>  	qsdev->input_buf = devm_kzalloc(qsdev->dev, max_input_len, GFP_KERNEL);
>  	if (!qsdev->input_buf)

[Severity: Medium]
This isn't a bug introduced by this patch, but should the transport drivers
be using `hid_safe_input_report()` instead of `hid_input_report()`?

According to HID subsystem guidelines, low-level transport drivers feeding raw
data from interrupt handlers should use the safe variant. Currently, both
`intel-quickspi` and `intel-quicki2c` use the unsafe API:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c:quickspi_hid_send_report() {
	ret = hid_input_report(qsdev->hid_dev, HID_INPUT_REPORT, data, data_len, 1);
}

Because the allocated size of the input buffer is not provided, the core
cannot strictly validate if the reported payload fits inside the backing buffer
allocation.


[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk during driver
teardown if an active IRQ handler executes concurrently?

During `quickspi_remove()`, the driver unconfigures the hardware and frees
the DMA allocations:

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_remove() {
	quickspi_dev_deinit(qsdev);
	quickspi_dma_deinit(qsdev);
}

However, the driver relies on `devm_request_threaded_irq()` for cleanup, which
only executes after the `remove` function returns. There appears to be no
synchronous wait (like `disable_irq()` or `free_irq()`) to stop a currently
running threaded handler.

If the handler is executing or preempted, could it resume and call
`thc_rxdma_read()`, accessing the now-freed DMA structures?

drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c:quickspi_irq_thread_handler() {
	ret = thc_rxdma_read(qsdev->thc_hw, THC_RXDMA2, qsdev->input_buf, &input_len, &read_finished);
}

The `intel-quicki2c` driver seems to have the exact same teardown sequence in
`quicki2c_remove()`. Does the IRQ handler need to be explicitly stopped before
freeing the DMA buffers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260820111558.475304-1-sammiee5311@gmail.com?part=1