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

[Xu, Even]

> -----Original Message-----
> From: Xu, Even
> Sent: Monday, September 14, 2026 10:00 AM
> To: 'HyeongJun An' <sammiee5311@xxxxxxxxx>; Sun, Xinpeng
> <Xinpeng.Sun@xxxxxxxxx>; Jiri Kosina <jikos@xxxxxxxxxx>; Benjamin Tissoires
> <bentiss@xxxxxxxxxx>
> Cc: linux-input@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> stable@xxxxxxxxxxxxxxx
> Subject: RE: [PATCH 2/2] HID: intel-thc-hid: intel-quickspi: bound the response to
> report_buf
> 
> 
> 
> > -----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

Recently, just realized a case:
THC hardware depends on touch controller's report length to determine the correct package length and buffer overflow.
If there is something wrong in the touch controller side, such as: touch controller's behavior is wrong, or data lost in bus transferring such as bus noise, THC hardware will receive wrong data.
We need to avoid this in software driver.
>From that viewpoint, this patch is valuable.

Reviewed-by: Even Xu <even.xu@xxxxxxxxx>

Hi, Jiri and Benjamin,

Could you help merge this patch?
Thank you very much!

Best Regards,
Even Xu

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