* Re: [PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy
2026-06-28 13:37 [PATCH] HID: intel-thc-hid: intel-quickspi: validate report size before copy HyeongJun An
@ 2026-06-28 13:51 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-28 13:51 UTC (permalink / raw)
To: HyeongJun An; +Cc: dmitry.torokhov, linux-input
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
^ permalink raw reply [flat|nested] 2+ messages in thread