[PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy

[PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy

[HyeongJun An]

write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
derived max_report_len (a few hundred bytes for a touch controller).  It
copies the caller-supplied report into that buffer:

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

The HID core caps a report at HID_MAX_BUFFER_SIZE (16384) by default, and
quickspi_hid_ll_driver does not set max_buffer_size, so the length reaches
the driver unbounded.  A hidraw SET_REPORT/SET_FEATURE ioctl carrying a
report larger than max_report_len therefore overflows report_buf with
attacker-controlled length and content.

Record the report_buf allocation size and reject reports that do not fit
before copying, matching the equivalent guard in the intel-quicki2c
sibling (quicki2c_init_write_buf()) and the hid-goodix-spi fix.

Fixes: 9d8d51735a3a ("HID: intel-thc-hid: intel-quickspi: Add HIDSPI protocol implementation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
 drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c      | 2 ++
 drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h      | 1 +
 drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c | 3 +++
 3 files changed, 6 insertions(+)

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 4ae2e1718b30..1695efd5961d 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -559,6 +559,8 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
 	if (!qsdev->report_buf)
 		return -ENOMEM;
 
+	qsdev->report_buf_size = max_report_len;
+
 	return 0;
 }
 
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index bf5e18f5a5f4..0ed964bfe3dd 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -157,6 +157,7 @@ struct quickspi_device {
 	u8 *report_descriptor;
 	u8 *input_buf;
 	u8 *report_buf;
+	u32 report_buf_size;
 	u32 report_len;
 
 	wait_queue_head_t reset_ack_wq;
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 cb19057f1191..db6054843e77 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
 
 	write_buf = (struct output_report *)qsdev->report_buf;
 
+	if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
+		return -EINVAL;
+
 	write_buf->output_hdr.report_type = report_type;
 	write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
 	write_buf->output_hdr.content_id = report_id;
-- 
2.43.0

Re: [PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy

[sashiko-bot]

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

New issues:
- [High] The newly introduced bounds check rejects valid, maximum-sized output reports.

Pre-existing issues:
- [Critical] Heap buffer overflow in GET_REPORT requests due to ignored user space length parameter.
- [High] Data races and waitqueue corruption due to concurrent lockless HID requests.
- [High] Heap buffer overflow in incoming report processing due to missing buffer bounds check.
- [High] Heap buffer overflow in `read_dma_buffer()` due to 4K max packet size rounding.
--

commit c642b7d6dc654ce88acbb51328c011ab2d5e04f0
Author: HyeongJun An <sammiee5311@gmail.com>

    HID: intel-thc-hid: intel-quickspi: validate report size before copy

    write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
    buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
    derived max_report_len. This patch records the report_buf allocation size
    and rejects reports that do not fit before copying.

> 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 4ae2e1718b30c..1695efd5961d7 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -559,6 +559,8 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
>  	if (!qsdev->report_buf)
>  		return -ENOMEM;
>  
> +	qsdev->report_buf_size = max_report_len;
> +

[Severity: High]
Since this size is based on the unadjusted device descriptor maximums, will
this reject valid maximum-sized output reports?

When the driver builds an output report in write_cmd_to_txdma(), it prepends
HIDSPI_OUTPUT_REPORT_HEADER_SIZE. If a maximum-sized report is sent, the
combined size will exceed qsdev->report_buf_size and trigger the new bounds
check.

>  	return 0;
>  }
>  

[ ... ]

> 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..db6054843e777 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> @@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
>  
>  	write_buf = (struct output_report *)qsdev->report_buf;
>  
> +	if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
> +		return -EINVAL;
> +
>  	write_buf->output_hdr.report_type = report_type;
>  	write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
>  	write_buf->output_hdr.content_id = report_id;
>  
>  	if (report_buf && report_buf_len > 0)
>  		memcpy(write_buf->content, report_buf, report_buf_len);

[Severity: High]
This is a pre-existing issue, but can multiple threads corrupt
qsdev->report_buf by executing this concurrently?

Since HID core does not serialize raw_request, multiple threads performing
concurrent HID ioctls (like HIDIOCGFEATURE and HIDIOCSFEATURE) could enter
write_cmd_to_txdma() simultaneously. Without locking, they would overwrite the
shared qsdev->report_buf with their respective headers and payloads.

This concurrent access could also corrupt the shared waitqueue state when
handling the responses:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    qsdev->get_report_cmpl = true;
    wake_up_interruptible(&qsdev->get_report_cmpl_wq);
    ...
}

[Severity: Critical]
This is a pre-existing issue, but does quickspi_get_report() overflow the
caller's buffer during GET_REPORT requests?

When a user space process issues a GET_REPORT ioctl with a smaller buffer,
hid_hw_raw_request() allocates a kernel buffer matching the requested length.
However, quickspi_hid_raw_request() discards this length parameter:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-hid.c:quickspi_hid_raw_request() {
    ...
    ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
    ...
}

Then quickspi_get_report() copies the full device-supplied response into this
potentially smaller buffer:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_get_report() {
    ...
    memcpy(buf, qsdev->report_buf, qsdev->report_len);
    ...
}

[Severity: High]
This is a pre-existing issue, but does quickspi_handle_input_data() lack
bounds checking before copying incoming reports?

When a device responds to a GET_FEATURE or GET_INPUT_REPORT, the function
trusts the incoming input_len:

drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    qsdev->report_len = sizeof(body_hdr->content_id) + input_len;
    input_report = input_body->content - sizeof(body_hdr->content_id);

    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
    ...
}

If a compromised or malicious SPI device sends an artificially large input_len
that fits within the DMA transaction's packet bounds (up to 4K) but exceeds
qsdev->report_buf_size, this memcpy will overflow the strictly allocated
qsdev->report_buf.

[Severity: High]
This is a pre-existing issue, but can the 4K packet size rounding in the DMA
configuration lead to a heap overflow?

dma_set_max_packet_size() rounds up the requested DMA max packet sizes (such
as max_input_len) to a 4K boundary:

drivers/hid/intel-thc-hid/intel-thc/intel-thc-dma.c:dma_set_max_packet_size() {
    ...
    dma_config->max_packet_size = ALIGN(size, SZ_4K);
    ...
}

When read_dma_buffer() receives data, it bounds-checks mes_len against this
4K-aligned max_packet_size before copying into the caller's buffer:

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);
    ...
}

If a malicious device returns more data than max_input_len but less than the
4K boundary, the check passes. Since qsdev->input_buf was allocated without
this 4K alignment, the copy will overflow the buffer.

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

[PATCH v2] HID: intel-thc-hid: intel-quickspi: validate report size before copy

[HyeongJun An]

write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
derived max_report_len (a few hundred bytes for a touch controller).  It
copies the caller-supplied report into that buffer:

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

The HID core caps a report at HID_MAX_BUFFER_SIZE (16384) by default, and
quickspi_hid_ll_driver does not set max_buffer_size, so the length reaches
the driver unbounded.  A hidraw SET_REPORT/SET_FEATURE ioctl carrying a
report larger than max_report_len therefore overflows report_buf with
attacker-controlled length and content.

Record the report_buf allocation size and reject reports that do not fit
before copying, matching the equivalent guard in the intel-quicki2c
sibling (quicki2c_init_write_buf()) and the hid-goodix-spi fix.

write_cmd_to_txdma() writes the output report header ahead of the content
in the same buffer, so size the allocation to cover the header as well.
That keeps the added bound from rejecting a maximum-sized report.

Fixes: 9d8d51735a3a ("HID: intel-thc-hid: intel-quickspi: Add HIDSPI protocol implementation")
Cc: stable@vger.kernel.org
Assisted-by: Claude:claude-opus-4-8
Signed-off-by: HyeongJun An <sammiee5311@gmail.com>
---
v2: Size report_buf to cover the output report header as well, so the added
    bound cannot reject a valid maximum-sized report (raised by the Sashiko
    AI review of v1).  No other change.

v1: https://lore.kernel.org/all/20260628133717.941389-1-sammiee5311@gmail.com/

 drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c  | 9 ++++++++-
 drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h  | 1 +
 .../hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c | 3 +++
 3 files changed, 12 insertions(+), 1 deletion(-)

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 4ae2e1718b30..da5ecfcd0fbf 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
@@ -555,7 +555,14 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
 	max_report_len = max(le16_to_cpu(qsdev->dev_desc.max_output_len),
 			     le16_to_cpu(qsdev->dev_desc.max_input_len));
 
-	qsdev->report_buf = devm_kzalloc(qsdev->dev, max_report_len, GFP_KERNEL);
+	/*
+	 * write_cmd_to_txdma() writes the output report header ahead of the
+	 * content in this buffer, so it has to hold both.
+	 */
+	qsdev->report_buf_size = HIDSPI_OUTPUT_REPORT_SIZE(max_report_len);
+
+	qsdev->report_buf = devm_kzalloc(qsdev->dev, qsdev->report_buf_size,
+					 GFP_KERNEL);
 	if (!qsdev->report_buf)
 		return -ENOMEM;
 
diff --git a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
index bf5e18f5a5f4..0ed964bfe3dd 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-dev.h
@@ -157,6 +157,7 @@ struct quickspi_device {
 	u8 *report_descriptor;
 	u8 *input_buf;
 	u8 *report_buf;
+	u32 report_buf_size;
 	u32 report_len;
 
 	wait_queue_head_t reset_ack_wq;
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 cb19057f1191..db6054843e77 100644
--- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
+++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
@@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
 
 	write_buf = (struct output_report *)qsdev->report_buf;
 
+	if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
+		return -EINVAL;
+
 	write_buf->output_hdr.report_type = report_type;
 	write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
 	write_buf->output_hdr.content_id = report_id;
-- 
2.43.0

Re: [PATCH v2] HID: intel-thc-hid: intel-quickspi: validate report size before copy

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unbounded memcpy in `quickspi_get_report()` causes a heap buffer overflow if the device responds with a report larger than the caller-allocated buffer.
- [High] The receive buffer `qsdev->input_buf` can be overflowed during an RXDMA interrupt because the message size is bounds-checked against a 4K-aligned limit instead of the actual buffer capacity.
- [High] Unbounded memcpy in `quickspi_handle_input_data()` causes a heap buffer overflow of `qsdev->report_buf` on the receive path.
- [High] Concurrent ioctl requests lead to data corruption in the shared DMA buffer `qsdev->report_buf`.
--

--- Patch [1]: [PATCH v2] HID: intel-thc-hid: intel-quickspi: validate report size before copy ---
commit 13f5051992cd9276077cab1966153d3866079f49
Author: HyeongJun An <sammiee5311@gmail.com>

    HID: intel-thc-hid: intel-quickspi: validate report size before copy
    
    write_cmd_to_txdma() builds an output report in qsdev->report_buf, a heap
    buffer allocated in quickspi_alloc_report_buf() to the device-descriptor
    derived max_report_len (a few hundred bytes for a touch controller).  It
    copies the caller-supplied report into that buffer:
    
[ ... ]

> 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 4ae2e1718b30c..da5ecfcd0fbf8 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/pci-quickspi.c
> @@ -555,7 +555,14 @@ static int quickspi_alloc_report_buf(struct quickspi_device *qsdev)
>  	max_report_len = max(le16_to_cpu(qsdev->dev_desc.max_output_len),
>  			     le16_to_cpu(qsdev->dev_desc.max_input_len));
>  
> -	qsdev->report_buf = devm_kzalloc(qsdev->dev, max_report_len, GFP_KERNEL);
> +	/*
> +	 * write_cmd_to_txdma() writes the output report header ahead of the
> +	 * content in this buffer, so it has to hold both.
> +	 */
> +	qsdev->report_buf_size = HIDSPI_OUTPUT_REPORT_SIZE(max_report_len);
> +
> +	qsdev->report_buf = devm_kzalloc(qsdev->dev, qsdev->report_buf_size,
> +					 GFP_KERNEL);

[Severity: High]
This is a pre-existing issue, but does the allocation for qsdev->input_buf
earlier in quickspi_alloc_report_buf() need to be 4K-aligned to prevent a heap
overflow during an RX DMA interrupt?

In thc_rxdma_read(), read_dma_buffer() bounds-checks the message length
against read_config->max_packet_size, which is 4K-aligned via
thc_dma_set_max_packet_sizes().

If a hardware device sends a DMA packet larger than max_input_len but
smaller than or equal to ALIGN(max_input_len, SZ_4K), it would pass the
bounds check in read_dma_buffer():

intel-thc/intel-thc-dma.c:read_dma_buffer() {
    ...
    if (mes_len > read_config->max_packet_size) {
    ...
}

and copy up to 4096 bytes into qsdev->input_buf, which was allocated strictly
to the unaligned max_input_len.

>  	if (!qsdev->report_buf)
>  		return -ENOMEM;
>  

[ ... ]

> 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..db6054843e777 100644
> --- a/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quickspi/quickspi-protocol.c
> @@ -30,6 +30,9 @@ static int write_cmd_to_txdma(struct quickspi_device *qsdev,
>  
>  	write_buf = (struct output_report *)qsdev->report_buf;
>  
> +	if (HIDSPI_OUTPUT_REPORT_SIZE(report_buf_len) > qsdev->report_buf_size)
> +		return -EINVAL;
> +
>  	write_buf->output_hdr.report_type = report_type;
>  	write_buf->output_hdr.content_len = cpu_to_le16(report_buf_len);
>  	write_buf->output_hdr.content_id = report_id;
>  
>  	if (report_buf && report_buf_len > 0)
>  		memcpy(write_buf->content, report_buf, report_buf_len);

[Severity: High]
This is a pre-existing issue, but can concurrent ioctl requests cause data
corruption in the shared DMA buffer qsdev->report_buf?

Multiple userspace processes can concurrently call HIDIOCSFEATURE or
HIDIOCGFEATURE. The HID core routines hold a read-lock, allowing parallel
execution in the driver callbacks.

Does write_cmd_to_txdma() modify qsdev->report_buf and trigger DMA writes
without mutual exclusion against concurrent TX or RX requests like
quickspi_handle_input_data()?

quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
    ...
}


[Severity: High]
This is a pre-existing issue, but can the receive path also overflow
qsdev->report_buf?

While this patch secures the output path, quickspi_handle_input_data()
parses input_len from the payload and executes an unbounded memcpy:

quickspi-protocol.c:quickspi_handle_input_data() {
    ...
    memcpy(qsdev->report_buf, input_report, qsdev->report_len);
    ...
}

If a compromised device sends a GET_FEATURE_RESPONSE with a length exceeding
qsdev->report_buf_size, will this cause a heap buffer overflow?


[Severity: Critical]
This isn't a bug introduced by this patch, but is there an unbounded memcpy
in quickspi_get_report() that could cause a heap buffer overflow?

When a userspace process calls ioctl(HIDIOCGFEATURE) with a length parameter
smaller than the actual hardware report size, quickspi_hid_raw_request()
discards the bounds-checking len parameter:

quickspi-hid.c:quickspi_hid_raw_request() {
    ...
    ret = quickspi_get_report(qsdev, rtype, reportnum, buf);
    ...
}

Then quickspi_get_report() copies qsdev->report_len bytes into the
caller's buffer without validating against the buffer's true capacity:

quickspi-protocol.c:quickspi_get_report() {
    ...
    memcpy(buf, qsdev->report_buf, qsdev->report_len);
    ...
}

Could this lead to memory corruption if the device responds with a report
larger than the caller-allocated buffer?

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