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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] Missing bounds check against allocated buffer size leads to a kernel heap buffer overflow.
- [High] Out-of-bounds write by 1 byte when copying the `HID_REQ_GET_REPORT` response in `spi_hid_ll_raw_request`.
- [High] Unsynchronized access to the shared `shid->response` buffer in `spi_hid_ll_raw_request`.
- [High] Timeout or interruption in `spi_hid_sync_request` leaves the device state and completion structure out-of-sync.
- [High] Device probe fails for standard HID devices with small reports because `spi_hid_ll_start` returns `-EINVAL`.
--

commit 475a21113f187310ac25983762798463f8e5115e
Author: Jingyuan Liang <jingyliang@chromium.org>

    HID: spi-hid: add spi-hid driver HID layer
    
    Add HID low level driver callbacks to register SPI as a HID driver, and
    an external touch device as a HID device.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index 02a7608c4b882..72c2e1ce3e8d9 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -84,11 +185,455 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state

[ ... ]

> +static int spi_hid_sync_request(struct spi_hid *shid,
> +				struct spi_hid_output_report *report)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	guard(mutex)(&shid->output_lock);
> +
> +	reinit_completion(&shid->output_done);
> +
> +	error = __spi_hid_send_output_report(shid, report);
> +	if (error)
> +		return error;
> +
> +	error = wait_for_completion_interruptible_timeout(&shid->output_done,
> +							  msecs_to_jiffies(SPI_HID_RESP_TIMEOUT));
> +	if (error == 0) {
> +		dev_err(dev, "Response timed out\n");
> +		return -ETIMEDOUT;
> +	}

[Severity: High]
If this times out or is interrupted, the function returns and releases
shid->output_lock, but does the hardware stop processing the request?

If the hardware is still running and sends a late response, could it
prematurely complete shid->output_done for a subsequent request, permanently
desynchronizing the driver and the device?

[ ... ]

> +static int spi_hid_alloc_buffers(struct spi_hid *shid, size_t report_size)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int inbufsize = round_up(sizeof(shid->input->header) +
> +				 sizeof(shid->input->body) + report_size, 4);
> +	int outbufsize = round_up(sizeof(shid->output->header) + report_size, 4);
> +	void *tmp;
> +
> +	tmp = devm_krealloc(dev, shid->output, outbufsize, GFP_KERNEL | __GFP_ZERO);
> +	if (!tmp)
> +		return -ENOMEM;
> +	shid->output = tmp;
> +
> +	tmp = devm_krealloc(dev, shid->input, inbufsize, GFP_KERNEL | __GFP_ZERO);
> +	if (!tmp)
> +		return -ENOMEM;
> +	shid->input = tmp;
> +
> +	tmp = devm_krealloc(dev, shid->response, inbufsize, GFP_KERNEL | __GFP_ZERO);

[Severity: Critical]
When allocating these buffers based on report_size, is there a corresponding
bounds check against the allocated size when reading from the SPI device?

In spi_hid_dev_irq() and spi_hid_input_sync(), the driver appears to read
header.report_length bytes into shid->input->body. Furthermore, in
spi_hid_response_handler(), memcpy copies body.content_len bytes into
shid->response->content.

Since these lengths are provided by the device, could a malicious or
malfunctioning device send a larger length and overflow the allocated heap
buffers?

[ ... ]

> +static int spi_hid_ll_start(struct hid_device *hid)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	int error = 0;
> +	u16 bufsize = 0;
> +
> +	spi_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
> +	spi_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
> +	spi_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
> +
> +	if (bufsize < HID_MIN_BUFFER_SIZE) {
> +		dev_err(&spi->dev,
> +			"HID_MIN_BUFFER_SIZE > max_input_length (%d)\n",
> +			bufsize);
> +		return -EINVAL;
> +	}

[Severity: High]
Does this check unconditionally reject standard HID devices (like mice or
keyboards) that legitimately have maximum report sizes smaller than
HID_MIN_BUFFER_SIZE (64 bytes)?

Should the driver enforce a minimum allocation size (for example, by padding
bufsize) instead of refusing to probe the device?

[ ... ]

> +static int spi_hid_ll_raw_request(struct hid_device *hid,
> +				  unsigned char reportnum, __u8 *buf,
> +				  size_t len, unsigned char rtype, int reqtype)
> +{
> +	struct spi_device *spi = hid->driver_data;
> +	struct spi_hid *shid = spi_get_drvdata(spi);
> +	struct device *dev = &spi->dev;
> +	int ret;
> +
> +	switch (reqtype) {

[ ... ]

> +	case HID_REQ_GET_REPORT:
> +		ret = spi_hid_get_request(shid, reportnum);

[Severity: High]
After spi_hid_get_request() returns, shid->output_lock is no longer held.
Could another thread concurrently call spi_hid_ll_raw_request() or
spi_hid_ll_output_report(), acquire the lock, and overwrite shid->response
before the current thread copies it?

> +		if (ret) {
> +			dev_err(dev, "failed to get report\n");
> +			return ret;
> +		}
> +
> +		ret = min_t(size_t, len,
> +			    (shid->response->body[1] | (shid->response->body[2] << 8)) + 1);
> +		buf[0] = shid->response->body[3];
> +		memcpy(&buf[1], &shid->response->content, ret);

[Severity: High]
Does this memcpy() cause an out-of-bounds write of 1 byte?

The code copies 'ret' bytes starting at '&buf[1]', which means it writes a
total of 'ret + 1' bytes into 'buf'. If 'ret' equals the allocated buffer
size 'len', it will write 'len + 1' bytes. Should the memcpy length
parameter be 'ret - 1'?

> +		break;
> +	default:
> +		dev_err(dev, "invalid request type\n");
> +		return -EIO;
> +	}
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-send-upstream-v4-0-b843d5e6ced3@chromium.org?part=4