Re: [PATCH 7/9] HID: intel-thc-hid: Drop redundant report ID validation in quicki2c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] hid: intel-thc-hid: hidraw GET_REPORT fails if buffer size exceeds report size
- [Medium] HID: intel-thc-hid: information leak via short DMA read
--

commit 75e0494a01241a3ea09c387003f59cc3e7199dc8
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: HID: intel-thc-hid: Drop redundant report ID validation in quicki2c

This commit removes the report ID validation from the Intel QuickI2C driver's
report retrieval path, as the core HID layer now handles this centrally. It
leaves the packet length validation intact.

> diff --git a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c
> index 41271301215ab..7a362a2ebe2b3 100644
> --- a/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c
> +++ b/drivers/hid/intel-thc-hid/intel-quicki2c/quicki2c-protocol.c
[ ... ]
> @@ -160,9 +160,9 @@ int quicki2c_get_report(struct quicki2c_device *qcdev, u8 report_type,
>  		return ret;
>  	}
>  
> -	if (HIDI2C_DATA_LEN(le16_to_cpu(rpt->len)) != buf_len || rpt->data[0] != reportnum) {
> -		dev_err_once(qcdev->dev, "Invalid packet, len (%d vs %zu) report id (%d vs %d)\n",
> -			     le16_to_cpu(rpt->len), buf_len, rpt->data[0], reportnum);
> +	if (HIDI2C_DATA_LEN(le16_to_cpu(rpt->len)) != buf_len) {

[Severity: High]
Does requiring an exact match for buf_len break the hidraw API? If a
userspace application calls HIDIOCGFEATURE with a standard oversized buffer
(e.g., HID_MAX_BUFFER_SIZE), buf_len will be larger than the actual report
size, causing this check to fail with -EINVAL.

[Severity: Medium]
Is there a risk of leaking stale data on a short DMA read?

If thc_swdma_read() successfully performs a short read, it returns 0 and
sets read_len to a value smaller than expected, leaving the rest of the DMA
buffer stale. Because this check exclusively relies on rpt->len instead of
the updated read_len, a stale rpt->len matching buf_len will pass this
validation.

This would lead to the memcpy() further down in quicki2c_get_report()
copying stale kernel memory into the buffer.

> +		dev_err_once(qcdev->dev, "Invalid packet length (%d vs %zu)\n",
> +			     le16_to_cpu(rpt->len), buf_len);
>  		return -EINVAL;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-hid-core-report-validation-v1-0-e0dd5cb60b2e@gmail.com?part=7