[PATCH 0/2] HID: intel-thc-hid: intel-quickspi: two DMA buffer overflows

[PATCH 0/2] HID: intel-thc-hid: intel-quickspi: two DMA buffer overflows

[HyeongJun An]

Two heap overflows on the QuickSPI receive path, both from a controller
that declares one length and sends another.

The first is the DMA landing buffer, sized from the device descriptor while
the THC DMA rounds its packet size up to 4K.  The second is the GET REPORT
response, whose length is checked against what the DMA delivered and never
against report_buf.  Each has a sibling in-tree that already gets it right,
named in the patch.

Neither was reproduced on hardware.  Both turned up while working on
commit 035ec4a71cb8 ("HID: intel-thc-hid: intel-quickspi: bound GET_REPORT
response to the caller buffer"), which covers neither.

A fuller fix for the first would give thc_rxdma_read() a capacity argument,
but that changes a shared API and touches intel-quicki2c, so this keeps to
the driver.

HyeongJun An (2):
  HID: intel-thc-hid: intel-quickspi: size the input buffer for the DMA
  HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to
    report_buf

 .../hid/intel-thc-hid/intel-quickspi/pci-quickspi.c  |  9 +++++++--
 .../intel-thc-hid/intel-quickspi/quickspi-protocol.c | 12 ++++++++++--
 2 files changed, 17 insertions(+), 4 deletions(-)

-- 
2.43.0

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

[HyeongJun An]

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.

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 89226f5ce45e..9ef33574c202 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -9,6 +9,7 @@
 #include <linux/interrupt.h>
 #include <linux/irqreturn.h>
 #include <linux/pci.h>
+#include <linux/sizes.h>
 #include <linux/pm_runtime.h>
 
 #include <linux/gpio/consumer.h>
@@ -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)
-- 
2.43.0

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

[sashiko-bot]

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

[PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf

[HyeongJun An]

quickspi_handle_input_data() copies a GET_FEATURE or GET_INPUT_REPORT
response into qsdev->report_buf using a length the controller supplied.
The only check it passes is against buf_len, the number of bytes the DMA
delivered, which says nothing about the destination.  report_buf holds
HIDSPI_OUTPUT_REPORT_SIZE(max(max_output_len, max_input_len)), 68 bytes
for a controller reporting 64 for both, while the copy is bounded only by
the 4K DMA packet.

The REPORT_DESCRIPTOR_RESPONSE case a few lines up validates against the
size of its own destination.  Do the same here and let the waiter in
quickspi_get_report() time out, as the other malformed-frame checks do.

Fixes: 9d8d51735a3a ("HID: intel-thc-hid: intel-quickspi: Add HIDSPI protocol implementation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-5
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
This needs commit a59cf84441f9 ("HID: intel-thc-hid: intel-quickspi:
validate report size before copy") for qsdev->report_buf_size, which is in
hid.git for-next but not mainline yet.  Same Fixes: tag, so a stable
backport wants the two in that order.

 .../intel-thc-hid/intel-quickspi/quickspi-protocol.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

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 847c5ec55569..7d34cc22a11a 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -161,6 +161,7 @@ void quickspi_handle_input_data(struct quickspi_device *qsdev, u32 buf_len)
 	struct input_report_body *input_body;
 	u8 *input_report;
 	u32 input_len;
+	u32 report_len;
 	int ret = 0;
 
 	input_body = (struct input_report_body *)qsdev->input_buf;
@@ -210,10 +211,17 @@ void quickspi_handle_input_data(struct quickspi_device *qsdev, u32 buf_len)
 
 	case GET_FEATURE_RESPONSE:
 	case GET_INPUT_REPORT_RESPONSE:
-		qsdev->report_len = sizeof(body_hdr->content_id) + input_len;
+		report_len = sizeof(body_hdr->content_id) + input_len;
+		if (report_len > qsdev->report_buf_size) {
+			dev_err_once(qsdev->dev, "Get report response too big: %u\n",
+				     report_len);
+			return;
+		}
+
+		qsdev->report_len = report_len;
 		input_report = input_body->content - sizeof(body_hdr->content_id);
 
-		memcpy(qsdev->report_buf, input_report, qsdev->report_len);
+		memcpy(qsdev->report_buf, input_report, report_len);
 
 		qsdev->get_report_cmpl = true;
 		wake_up_interruptible(&qsdev->get_report_cmpl_wq);
-- 
2.43.0

Re: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf

[sashiko-bot]

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

Pre-existing issues:
- [High] State machine desynchronization in raw request handlers due to unreset completion flags.
- [High] Missing locking in raw request handlers allows concurrent requests to corrupt the shared command and response buffers.
- [High] Use-After-Free: DMA buffers are freed during driver remove() without synchronizing with the threaded IRQ handler.
- [Medium] Subsystem API Violation: The transport driver uses hid_input_report() instead of hid_safe_input_report().
- [Medium] Signal interruptions are incorrectly treated as timeouts in raw requests, breaking signal handling.
--

--- Patch [2]: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf ---
commit c4e25ca5cbdc0b92b00b6a64c016c4b8ee39d40d
Author: HyeongJun An <sammiee5311@gmail.com>

HID: intel-thc-hid: intel-quickspi: bound the GET REPORT response to report_buf

This patch bounds the GET_FEATURE and GET_INPUT_REPORT response copy lengths
to qsdev->report_buf_size. It drops responses that exceed this size to prevent
overflowing the buffer with responses bounded only by the DMA packet size.

> 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 847c5ec55569a..7d34cc22a11a8 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
[ ... ]
> @@ -210,10 +211,17 @@ void quickspi_handle_input_data(struct quickspi_device *qsdev, u32 buf_len)
>  
>  	case GET_FEATURE_RESPONSE:
>  	case GET_INPUT_REPORT_RESPONSE:
> -		qsdev->report_len = sizeof(body_hdr->content_id) + input_len;
> +		report_len = sizeof(body_hdr->content_id) + input_len;
> +		if (report_len > qsdev->report_buf_size) {
> +			dev_err_once(qsdev->dev, "Get report response too big: %u\n",
> +				     report_len);
> +			return;
> +		}
> +
> +		qsdev->report_len = report_len;
>  		input_report = input_body->content - sizeof(body_hdr->content_id);
>  
> -		memcpy(qsdev->report_buf, input_report, qsdev->report_len);
> +		memcpy(qsdev->report_buf, input_report, report_len);

[Severity: High]
This is a pre-existing issue, but missing locking in the raw request handlers 
appears to allow concurrent requests to corrupt qsdev->report_buf.

The HID core does not serialize raw_request callbacks. If multiple threads 
concurrently call quickspi_hid_raw_request(), they will both execute 
write_cmd_to_txdma(). 

This function writes the outgoing command into a single, unprotected shared 
buffer qsdev->report_buf:

quickspi_hid_raw_request() -> write_cmd_to_txdma() {
    ...
    write_buf = (struct output_report *)qsdev->report_buf;
    ...
    memcpy(write_buf->content, report_buf, report_buf_len);
}

Concurrently, the IRQ handler quickspi_handle_input_data() also writes the 
incoming response directly into the exact same qsdev->report_buf here.

Without a mutex protecting the request-response cycle and this buffer, could 
concurrent requests overwrite each other's outgoing commands, and could 
incoming responses overwrite commands being constructed?


[Severity: High]
This is a pre-existing issue, but can the state machine become desynchronized 
if a raw request times out?

If quickspi_get_report() times out, it returns -ETIMEDOUT without resetting 
the completion flag qsdev->get_report_cmpl:

quickspi_get_report() {
    ...
    ret = wait_event_interruptible_timeout(qsdev->get_report_cmpl_wq,
                                           qsdev->get_report_cmpl,
                                           QUICKSPI_ACK_WAIT_TIMEOUT * HZ);
    ...
}

If the hardware eventually sends a delayed response, the IRQ handler will 
set the flag to true. When a subsequent raw request is issued, it will write 
the new command, but wait_event_interruptible_timeout() will return 
immediately because the flag is already true.

Would this cause the driver to return stale data from the previous request 
and lose synchronization with the device? 

The set_report_cmpl flag appears to suffer from the exact same 
desynchronization flaw in quickspi_set_report().


[Severity: Medium]
This is a pre-existing issue, but are signal interruptions incorrectly treated 
as timeouts in raw requests?

In quickspi_get_report() and quickspi_set_report(), the driver uses 
wait_event_interruptible_timeout(). If the wait is interrupted by a signal, 
the function returns a negative error code (such as -ERESTARTSYS).

The driver checks for ret <= 0 and unconditionally returns -ETIMEDOUT:

quickspi_get_report() {
    ...
    if (ret <= 0 || !qsdev->get_report_cmpl) {
        dev_err_once(qsdev->dev, "Wait Get Report Response timeout, ret:%d\n", ret);
        return -ETIMEDOUT;
    }
    ...
}

Does this swallow the actual signal error code, preventing userspace from 
correctly handling signals and breaking the syscall restart mechanism?


[Severity: Medium]
This is a pre-existing issue, but should the transport driver use 
hid_safe_input_report() instead of hid_input_report() here?

According to the HID subsystem documentation, transport drivers must prefer 
hid_safe_input_report() over hid_input_report() when feeding data from 
hardware interrupts to the HID core. 

This allows the core to perform strict boundary checks against the maximum 
allocated buffer size, preventing out-of-bounds reads if a device supplies 
a malicious report descriptor.

quickspi_hid_send_report() {
    ...
    ret = hid_input_report(qsdev->hid_dev, HID_INPUT_REPORT, data, data_len, 1);
    ...
}

Does using hid_input_report() expose the HID core to potential out-of-bounds 
reads?


[Severity: High]
This is a pre-existing issue, but could DMA buffers be freed during driver 
remove() without synchronizing with the threaded IRQ handler?

During driver removal, quickspi_remove() disables hardware interrupts and 
then calls quickspi_dma_deinit() which releases the DMA buffers:

quickspi_remove() {
    ...
    quickspi_dev_deinit(qsdev);
    quickspi_dma_deinit(qsdev);
    ...
}

However, it does not seem to synchronize with the devm-managed threaded IRQ 
handler (quickspi_irq_thread_handler), which might already be executing on 
another CPU. 

The threaded handler calls thc_rxdma_read(), which reads from the DMA 
buffers:

quickspi_irq_thread_handler() {
    ...
    ret = thc_rxdma_read(qsdev->thc_hw, THC_RXDMA2, qsdev->input_buf,
                         &input_len, &read_finished);
    ...
}

If thc_dma_release() frees these buffers while the threaded handler is 
concurrently executing, could this result in a Use-After-Free?

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