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

[Xu, Even]

> -----Original Message-----
> From: HyeongJun An <sammiee5311@xxxxxxxxx>
> Sent: Thursday, August 20, 2026 7:16 PM
> To: Xu, Even <even.xu@xxxxxxxxx>; Sun, Xinpeng <xinpeng.sun@xxxxxxxxx>; Jiri
> Kosina <jikos@xxxxxxxxxx>; Benjamin Tissoires <bentiss@xxxxxxxxxx>
> Cc: linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx; HyeongJun An <sammiee5311@xxxxxxxxx>
> Subject: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the response to
> report_buf
> 
> 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

This is a special capability of the THC hardware. At the hardware level, THC supports the HID-over-SPI protocol, which means it understands every HIDSPI message it receives. When THC initiates a DMA transfer to fetch data, it verifies whether the received data size, including both the packet size declared in the header and the actual size of the transmitted packet, complies with the MAX Packet Size requirement. The packet is accepted only if it does. If the size exceeds the limit, THC reports a DMA buffer overflow, and the packet is discarded.

This capability of handling software logic in hardware can greatly simplify the driver's processing logic and maximize data-transfer performance. This is also why Intel created the THC IP.

> 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@xxxxxxxxxxxxxxx
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: HyeongJun An <sammiee5311@xxxxxxxxx>
> ---
> 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);

According to above explanation, this logic checking isn't a problem, because THC hardware will check it and guarantee the safe size.

Thanks for your patch!

Best Regards,
Even Xu

> 
>  		qsdev->get_report_cmpl = true;
>  		wake_up_interruptible(&qsdev->get_report_cmpl_wq);
> --
> 2.43.0