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

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 [off-list ref]

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?

[ ... ]

[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