Re: [PATCH v6 05/11] HID: spi-hid: add HID SPI protocol implementation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] hid: spi-hid: spi_hid_bus_validate_header() rejects descriptors > max_input
- [High] hid: spi-hid: spi_hid_dev_desc_response() clears reset_attempts too early
- [Medium] hid: spi-hid: spi_hid_bus_validate_header() allows 0-length report body
- [Medium] hid: spi-hid: stale flags and duplicate SPI_HID_ERROR cause spurious resets
- [Medium] hid: spi-hid: unaborted sync request in __spi_hid_sync_request() on reset
--

commit 68d1db2b55c0e004ec2c06854ae93db414465298
Author: Jingyuan Liang <jingyliang@chromium.org>

HID: spi-hid: add HID SPI protocol implementation

This commit implements the HID Over SPI Protocol Specification 1.0 in
drivers/hid/spi-hid/spi-hid-core.c, handling SPI input/output transfers,
descriptor parsing, interrupt processing, and error recovery resets.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> index fca7a44eeb9f9..90b2c2fea6501 100644
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c

[ ... ]

> @@ -187,17 +350,94 @@ static const char *spi_hid_power_mode_string(enum hidspi_power_state power_state
>  
>  static void spi_hid_stop_hid(struct spi_hid *shid)
>  {
> -	struct hid_device *hid = shid->hid;
> +	struct hid_device *hid;
>  
> -	shid->hid = NULL;
> -	clear_bit(SPI_HID_READY, &shid->flags);
> +	scoped_guard(mutex, &shid->io_lock) {
> +		hid = shid->hid;
> +		shid->hid = NULL;
> +		clear_bit(SPI_HID_READY, &shid->flags);
> +	}
>  
>  	if (hid)
>  		hid_destroy_device(hid);
>  }
>  
> +static void spi_hid_error_handler(struct spi_hid *shid)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	int error;
> +
> +	guard(mutex)(&shid->power_lock);
> +	if (shid->power_state == HIDSPI_OFF)
> +		return;
> +
> +	disable_irq(shid->spi->irq);
> +
> +	if (shid->reset_attempts++ >= SPI_HID_MAX_RESET_ATTEMPTS) {
> +		dev_err(dev, "unresponsive device, aborting\n");
> +		spi_hid_stop_hid(shid);
> +		shid->ops->assert_reset(shid->ops);
> +		error = shid->ops->power_down(shid->ops);
> +		if (error) {
> +			dev_err(dev, "failed to disable regulator\n");
> +			shid->regulator_error_count++;
> +			shid->regulator_last_error = error;
> +		}
> +		shid->power_state = HIDSPI_OFF;
> +		/* Dead device: leave the IRQ permanently disabled. */
> +		return;
> +	}
> +
> +	clear_bit(SPI_HID_READY, &shid->flags);
> +	set_bit(SPI_HID_RESET_PENDING, &shid->flags);
> +
> +	shid->ops->assert_reset(shid->ops);
> +
> +	shid->power_state = HIDSPI_OFF;
> +
> +	/*
> +	 * We want to cancel pending reset work as the device is being reset
> +	 * to recover from an error. cancel_work_sync will put us in a deadlock
> +	 * because this function is scheduled in 'reset_work' and we should
> +	 * avoid waiting for itself.
> +	 */
> +	cancel_work(&shid->reset_work);

[Severity: Medium]
Should spi_hid_error_handler() also clear pending SPI_HID_RESET_RESPONSE,
SPI_HID_CREATE_DEVICE, and SPI_HID_ERROR bits in shid->flags here?

Furthermore, in __spi_hid_sync_request(), a timeout already sets
SPI_HID_ERROR and schedules shid->reset_work, but its callers
(spi_hid_reset_response(), spi_hid_report_descriptor_request(), and
spi_hid_get_request()) unconditionally set SPI_HID_ERROR and schedule
shid->reset_work a second time on error:

  spi_hid_get_request()
    __spi_hid_sync_request()
      set_bit(SPI_HID_ERROR, &shid->flags);
      schedule_work(&shid->reset_work);
      return -ETIMEDOUT;
    set_bit(SPI_HID_ERROR, &shid->flags);
    schedule_work(&shid->reset_work);

If shid->reset_work starts on another CPU after the first schedule_work()
and clears SPI_HID_ERROR via test_and_clear_bit(), the caller can set
SPI_HID_ERROR and queue shid->reset_work again. If cancel_work() cancels
the work item without clearing the flag bits, the stale SPI_HID_ERROR bit
remains set in shid->flags and will trigger a second reset when
spi_hid_reset_work() later checks pending flags to reschedule itself. And
if the second schedule_work() runs after cancel_work(), reset_work will run
a second time immediately.

Also, if spi_hid_get_request() is called while SPI_HID_READY is cleared,
__spi_hid_send_output_report() returns -ENODEV, which spi_hid_get_request()
treats as a fatal error by setting SPI_HID_ERROR and scheduling a hardware
reset.

Finally, in spi_hid_reset_work(), SPI_HID_RESET_RESPONSE and
SPI_HID_CREATE_DEVICE are checked before SPI_HID_ERROR in the if/else-if
chain, so if SPI_HID_ERROR is set while one of those bits is pending, the
worker will attempt normal initialization before running
spi_hid_error_handler().

> +
> +	shid->ops->sleep_minimal_reset_delay(shid->ops);
> +
> +	shid->power_state = HIDSPI_ON;
> +
> +	shid->ops->deassert_reset(shid->ops);
> +
> +	enable_irq(shid->spi->irq);
> +}

[ ... ]

> @@ -234,39 +491,208 @@ static int spi_hid_send_output_report(struct spi_hid *shid,
>  				      struct spi_hid_output_report *report)
>  {
>  	guard(mutex)(&shid->output_lock);
> -	return __spi_hid_send_output_report(shid, report);
> +	return __spi_hid_send_output_report(shid, report, 0);
>  }
>  
>  static int __spi_hid_sync_request(struct spi_hid *shid,
>  				  struct spi_hid_output_report *report)
>  {
>  	struct device *dev = &shid->spi->dev;
>  	int error;
>  
> -	reinit_completion(&shid->output_done);
> -
> -	error = __spi_hid_send_output_report(shid, report);
> +	error = __spi_hid_send_output_report(shid, report,
> +					     spi_hid_response_type(report->report_type));
>  	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;
> +	if (wait_for_completion_timeout(&shid->output_done,
> +					msecs_to_jiffies(SPI_HID_RESP_TIMEOUT)))
> +		return 0;
> +
> +	/* Drop late responses and block new HID requests until the reset. */
> +	scoped_guard(mutex, &shid->io_lock) {
> +		shid->expected_response = 0;
> +		clear_bit(SPI_HID_READY, &shid->flags);
>  	}
> -	if (error < 0)
> -		return error;
>  
> -	return 0;
> +	dev_err(dev, "Response timed out\n");
> +	/* The device may still answer later: resync it with a reset. */
> +	set_bit(SPI_HID_ERROR, &shid->flags);
> +	schedule_work(&shid->reset_work);
> +	return -ETIMEDOUT;
>  }

[Severity: Medium]
What happens if a spontaneous firmware reset (RESET_RESPONSE in
spi_hid_process_input_report()) or an IRQ error (spi_hid_error_handler())
occurs while a synchronous request is waiting on shid->output_done in
__spi_hid_sync_request() with shid->output_lock held?

Because neither the RESET_RESPONSE path nor spi_hid_error_handler() aborts
the in-flight request or wakes shid->output_done, the waiter continues
holding shid->output_lock for the full SPI_HID_RESP_TIMEOUT (blocking
spi_hid_reset_response() in reset_work) and then sets SPI_HID_ERROR and
schedules reset_work when the timeout expires, resetting the device a
second time after recovery.

[ ... ]

> +static int spi_hid_dev_desc_response(struct spi_hid *shid,
> +				     struct spi_hid_input_report *body)
> +{
> +	struct hidspi_dev_descriptor *raw =
> +		(struct hidspi_dev_descriptor *)shid->input->content;
> +	struct device *dev = &shid->spi->dev;
> +
> +	/* Validate device descriptor length before parsing */
> +	if (body->content_length != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> +		dev_err(dev, "Invalid content length %d, expected %zu\n",
> +			body->content_length, HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> +		return -EPROTO;
> +	}
> +
> +	if (le16_to_cpu(raw->dev_desc_len) != HIDSPI_DEVICE_DESCRIPTOR_SIZE) {
> +		dev_err(dev, "Invalid wDeviceDescLength %d, expected %zu\n",
> +			le16_to_cpu(raw->dev_desc_len),
> +			HIDSPI_DEVICE_DESCRIPTOR_SIZE);
> +		return -EPROTO;
> +	}
> +
> +	if (le16_to_cpu(raw->bcd_ver) != SPI_HID_SUPPORTED_VERSION) {
> +		dev_err(dev, "Unsupported device descriptor version %4x\n",
> +			le16_to_cpu(raw->bcd_ver));
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	/* Fail before reset_attempts is cleared so resets stay bounded. */
> +	if (!le16_to_cpu(raw->rep_desc_len) ||
> +	    le16_to_cpu(raw->rep_desc_len) > HID_MAX_DESCRIPTOR_SIZE) {
> +		dev_err(dev, "Invalid wReportDescLength %d, max %d\n",
> +			le16_to_cpu(raw->rep_desc_len),
> +			HID_MAX_DESCRIPTOR_SIZE);
> +		return -EPROTO;
> +	}
> +
> +	spi_hid_parse_dev_desc(raw, &shid->desc);
> +
> +	/* Reset attempts at every device descriptor fetch */
> +	shid->reset_attempts = 0;
> +	set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
> +	schedule_work(&shid->reset_work);
> +
> +	return 0;
> +}

[Severity: High]
Does clearing shid->reset_attempts = 0 here in spi_hid_dev_desc_response()
cause an unbounded reset loop if fetching or validating the report
descriptor subsequently fails?

When SPI_HID_CREATE_DEVICE runs spi_hid_create_device() -> hid_add_device()
-> spi_hid_ll_parse() (or spi_hid_refresh_device()), it calls
spi_hid_report_descriptor_request(). If that request times out or fails
validation in spi_hid_dev_irq(), SPI_HID_ERROR is set and
spi_hid_error_handler() increments shid->reset_attempts from 0 to 1 and
resets the device.

After the reset, if the device responds to DEVICE_DESCRIPTOR again,
spi_hid_dev_desc_response() resets shid->reset_attempts back to 0 before
REPORT_DESCRIPTOR is retried, so shid->reset_attempts oscillates between 0
and 1 and never reaches SPI_HID_MAX_RESET_ATTEMPTS. Should
shid->reset_attempts only be cleared once the report descriptor is fetched
and SPI_HID_READY is set?

[ ... ]

> @@ -334,13 +766,205 @@ static int spi_hid_create_device(struct spi_hid *shid)

[ ... ]

> +static int spi_hid_bus_validate_header(struct spi_hid *shid,
> +				       struct spi_hid_input_header *header)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	/*
> +	 * Body capacity of shid->input, matching spi_hid_alloc_buffers().
> +	 * shid->bufsize only changes with the IRQ disabled, so it is stable here.
> +	 */
> +	u32 max_body = round_up(sizeof(shid->input->header) +
> +				sizeof(shid->input->body) + shid->bufsize, 4) -
> +		       sizeof(shid->input->header);
> +
> +	if (header->version != SPI_HID_INPUT_HEADER_VERSION) {
> +		dev_err(dev, "Unknown input report version (v 0x%x)\n",
> +			header->version);
> +		return -EINVAL;
> +	}
> +
> +	/*
> +	 * report_length is device-provided and is used as the size of the body
> +	 * transfer, so it must never exceed the input buffer.
> +	 */
> +	if (header->report_length > max_body) {
> +		dev_err(dev, "Input report too big: %u > %u\n",
> +			header->report_length, max_body);
> +		return -EMSGSIZE;
> +	}

[Severity: Medium]
Can header->report_length be 0 (or smaller than sizeof(shid->input->body))
here?

In spi_hid_populate_input_header(), header->report_length is computed as
(get_unaligned_le16(&buf[1]) & 0x3fff) * 4. If a malformed header or bus
glitch during reset produces a length of 0, spi_hid_bus_validate_header()
returns 0 because it only checks the upper bound.

In spi_hid_dev_irq(), that bypasses the SPI_HID_RESET_PENDING check,
performs a 0-byte body read via spi_hid_input_sync(), and then parses stale
bytes from shid->input->body in spi_hid_process_input_report(), triggering
spi_hid_irq_error(). Should spi_hid_bus_validate_header() reject
header->report_length < sizeof(shid->input->body)?

> +
> +	if (shid->desc.max_input_length != 0 &&
> +	    header->report_length > shid->desc.max_input_length) {
> +		dev_err(dev, "Input report body size %u > max expected of %u\n",
> +			header->report_length, shid->desc.max_input_length);
> +		return -EMSGSIZE;
> +	}

[Severity: High]
Won't this check in spi_hid_bus_validate_header() reject
REPORT_DESCRIPTOR_RESPONSE (and post-reset DEVICE_DESCRIPTOR_RESPONSE)
whenever the descriptor packet is larger than shid->desc.max_input_length?

During initialization:

  spi_hid_dev_desc_response()
    spi_hid_parse_dev_desc(raw, &shid->desc)
      desc->max_input_length = le16_to_cpu(raw->max_input_len);
    set_bit(SPI_HID_CREATE_DEVICE, &shid->flags);
    schedule_work(&shid->reset_work);

When spi_hid_reset_work() then calls spi_hid_create_device() ->
hid_add_device() -> spi_hid_ll_parse() ->
spi_hid_report_descriptor_request(), shid->desc.max_input_length is already
populated with wMaxInputLength.

When the device replies with REPORT_DESCRIPTOR_RESPONSE, spi_hid_dev_irq()
calls spi_hid_bus_validate_header() before reading the body or knowing the
report type. Since wMaxInputLength is the maximum length of input reports
while wReportDescLength (up to HID_MAX_DESCRIPTOR_SIZE = 4096) is typically
much larger, header->report_length > shid->desc.max_input_length will fail
with -EMSGSIZE and trigger spi_hid_irq_error() on every attempt until the
device is aborted. (Note also the comment in spi_hid_find_max_report() that
some devices set wMaxInputLength to an inaccurate length.)

> +
> +	if (header->last_fragment_flag != 1) {
> +		dev_err(dev, "Multi-fragment reports not supported\n");
> +		return -EOPNOTSUPP;
> +	}

[ ... ]

> @@ -365,6 +989,8 @@ static int spi_hid_get_request(struct spi_hid *shid, u8 content_id,
>  		dev_err(dev,
>  			"Expected get request response not received! Error %d\n",
>  			error);
> +		set_bit(SPI_HID_ERROR, &shid->flags);
> +		schedule_work(&shid->reset_work);
>  		return error;
>  	}

[ ... ]

> @@ -397,9 +1023,81 @@ static int spi_hid_set_request(struct spi_hid *shid, u8 *arg_buf, u16 arg_len,
>  	return spi_hid_sync_request(shid, &report);
>  }
>  
> -/* This is a placeholder. Will be implemented in the next patch. */
> +/* Schedule a reset to recover from an error in the IRQ handler. */
> +static irqreturn_t spi_hid_irq_error(struct spi_hid *shid)
> +{
> +	set_bit(SPI_HID_ERROR, &shid->flags);
> +	schedule_work(&shid->reset_work);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static irqreturn_t spi_hid_dev_irq(int irq, void *_shid)
>  {
> +	struct spi_hid *shid = _shid;
> +	struct device *dev = &shid->spi->dev;
> +	struct spi_hid_input_header header;
> +	int error = 0;
> +
> +	scoped_guard(mutex, &shid->io_lock) {
> +		if (shid->power_state == HIDSPI_OFF) {
> +			dev_warn(dev, "Device is off, ignoring interrupt\n");
> +			return IRQ_NONE;
> +		}
> +
> +		error = spi_hid_input_sync(shid, shid->input->header,
> +					   sizeof(shid->input->header), true);
> +		if (error) {
> +			dev_err(dev, "Failed to transfer header: %d\n", error);
> +			return spi_hid_irq_error(shid);
> +		}
> +
> +		if (shid->input_message.status < 0) {
> +			dev_warn(dev, "Error reading header: %d\n",
> +				 shid->input_message.status);
> +			shid->bus_error_count++;
> +			shid->bus_last_error = shid->input_message.status;
> +			return spi_hid_irq_error(shid);
> +		}
> +
> +		spi_hid_populate_input_header(shid->input->header, &header);
> +
> +		error = spi_hid_bus_validate_header(shid, &header);
> +		if (error) {
> +			if (!test_bit(SPI_HID_RESET_PENDING, &shid->flags)) {
> +				dev_err(dev, "Failed to validate header: %d\n", error);
> +				print_hex_dump(KERN_ERR, "spi_hid: header buffer: ",
> +					       DUMP_PREFIX_NONE, 16, 1, shid->input->header,
> +					       sizeof(shid->input->header), false);
> +				shid->bus_error_count++;
> +				shid->bus_last_error = error;
> +				return spi_hid_irq_error(shid);
> +			}
> +			return IRQ_HANDLED;
> +		}
> +
> +		error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
> +					   false);

[ ... ]

> @@ -479,6 +1177,8 @@ static int spi_hid_ll_start(struct hid_device *hid)

[ ... ]

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