Re: [PATCH v6 04/11] HID: spi-hid: add spi-hid driver HID layer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] HID: spi-hid: zero-length report descriptor accepted in spi_hid_ll_parse()
--

commit 9328ec8c48a0b55cf1019d845ec2c37e89f4cdbb
Author: Jingyuan Liang <jingyliang@chromium.org>

HID: spi-hid: add spi-hid driver HID layer

This commit adds the HID low-level driver callbacks and device creation logic
for spi-hid, handling report descriptor fetching, buffer allocation, and
output/feature report requests.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..fca7a44eeb9f9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> @@ -84,11 +185,484 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
[ ... ]
> +/*
> + * This function returns the length of the report descriptor, or a negative
> + * error code if something went wrong.
> + */
> +static int spi_hid_report_descriptor_request(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	struct spi_hid_output_report report = {
> +		.report_type = REPORT_DESCRIPTOR,
> +		.content_length = 0,
> +		.content_id = SPI_HID_OUTPUT_REPORT_CONTENT_ID_DESC_REQUEST,
> +		.content = NULL,
> +	};
> +	int ret;
> +
> +	ret =  spi_hid_sync_request(shid, &report);
> +	if (ret) {
> +		dev_err(dev,
> +			"Expected report descriptor not received: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = shid->response_length;
> +	if (ret != shid->desc.report_descriptor_length) {
> +		ret = min_t(unsigned int, ret, shid->desc.report_descriptor_length);
> +		dev_err(dev, "Received report descriptor length doesn't match device descriptor field, using min of the two: %d\n",
> +			ret);
> +	}
> +
> +	return ret;
> +}
[ ... ]
> +static int spi_hid_ll_parse(struct hid_device *hid)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	struct device *dev = &spi->dev;
> +	unsigned int rsize = shid->desc.report_descriptor_length;
> +	int error, len;
> +
> +	if (rsize > HID_MAX_DESCRIPTOR_SIZE) {
> +		dev_err(dev,
> +			"Report descriptor size %d is greater than HID_MAX_DESCRIPTOR_SIZE %d\n",
> +			rsize, HID_MAX_DESCRIPTOR_SIZE);
> +		return -EINVAL;
> +	}
> +
> +	if (rsize > shid->bufsize) {
> +		error = spi_hid_alloc_buffers(shid, rsize);
> +		if (error)
> +			return error;
> +	}
> +
> +	len = spi_hid_report_descriptor_request(shid);
> +	if (len < 0) {
> +		dev_err(dev, "Report descriptor request failed, %d\n", len);
> +		return len;
> +	}
> +
> +	/*
> +	 * FIXME: below call returning 0 doesn't mean that the report descriptor
> +	 * is good. We might be caching a crc32 of a corrupted r. d. or who
> +	 * knows what the FW sent. Need to have a feedback loop about r. d.
> +	 * being ok and only then cache it.
> +	 */
> +	error = hid_parse_report(hid, (u8 *)shid->response->content, len);
> +	if (error) {
> +		dev_err(dev, "failed parsing report: %d\n", error);
> +		return error;
> +	}
> +	shid->report_descriptor_crc32 = crc32_le(0,
> +						 (unsigned char const *)shid->response->content,
> +						 len);
> +
> +	set_bit(SPI_HID_READY, &shid->flags);
> +
> +	return 0;
> +}

[Severity: Medium]
What happens in spi_hid_ll_parse() if the report descriptor length is 0?

In spi_hid_ll_parse(), rsize (shid->desc.report_descriptor_length) is checked
against HID_MAX_DESCRIPTOR_SIZE, but not for zero. Later in the series, the
commit "HID: spi-hid: add HID SPI protocol implementation" validates
raw->rep_desc_len != 0 in spi_hid_dev_desc_response(), but if a device returns
a REPORT_DESCRIPTOR_RESPONSE with content_length == 0 (shid->response_length ==
0), spi_hid_report_descriptor_request() computes min_t(unsigned int, 0,
shid->desc.report_descriptor_length) and returns 0.

Both spi_hid_ll_parse() here and spi_hid_refresh_device() in the protocol patch
only check if (len < 0), treating len == 0 as success.

When len == 0 is passed to hid_parse_report():

drivers/hid/hid-core.c:hid_parse_report() {
	hid->dev_rdesc = kmemdup(start, size, GFP_KERNEL);
	if (!hid->dev_rdesc)
		return -ENOMEM;
	hid->dev_rsize = size;
	return 0;
}

kmemdup() returns ZERO_SIZE_PTR (non-NULL), so hid_parse_report() returns 0.
spi_hid_ll_parse() then caches a CRC32 of 0 and sets SPI_HID_READY, and
hid_add_device() sees a non-NULL hdev->dev_rdesc and calls device_add(),
registering an unusable hid_device while bypassing SPI_HID_ERROR reset
recovery.

Should spi_hid_report_descriptor_request() or its callers reject a 0-length
report descriptor (for example, checking !rsize in spi_hid_ll_parse() and
returning -EINVAL when shid->response_length == 0 or len <= 0)?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019-send-upstream-v6-0-cb9ba4b3d6fa@chromium.org?part=4