Re: [PATCH v4 08/11] HID: spi_hid: add device tree support for SPI over HID

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Heap buffer overflow via attacker-controlled `report_length`.
- [Medium] API misuse: Calling `gpiod_set_value` instead of `gpiod_set_value_cansleep`.
- [Medium] Hardware backpowering risk due to improper GPIO initialization.
- [Low] Log spam during boot due to unhandled -EPROBE_DEFER.
--

commit 0c5bcceb42b14d25affd5a3652cde4d4a22e95c2
Author: Jarrett Schultz <jaschultz@microsoft.com>

HID: spi_hid: add device tree support for SPI over HID

This commit adds Open Firmware (Device Tree) transport support for HID
over SPI devices. It introduces the spi_hid_of module to configure and
manage power and reset lines for these devices based on device tree
properties.

> diff --git a/drivers/hid/spi-hid/spi-hid-of.c b/drivers/hid/spi-hid/spi-hid-of.c
> new file mode 100644
> index 0000000000000..ba7d5338f5d89
> --- /dev/null
> +++ b/drivers/hid/spi-hid/spi-hid-of.c
> @@ -0,0 +1,246 @@

[ ... ]

> +static int spi_hid_of_populate_config(struct spi_hid_of_config *conf,
> +				      struct device *dev)
> +{
> +	int error;
> +	u32 val;

[ ... ]

> +	conf->supply = devm_regulator_get(dev, "vdd");
> +	if (IS_ERR(conf->supply)) {
> +		if (PTR_ERR(conf->supply) != -EPROBE_DEFER)
> +			dev_err(dev, "Failed to get regulator: %ld.",
> +				PTR_ERR(conf->supply));
> +		return PTR_ERR(conf->supply);
> +	}
> +	conf->supply_enabled = false;
> +
> +	conf->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_OUT_LOW);

[Severity: Medium]
Could initializing the reset GPIO with GPIOD_OUT_LOW cause hardware
backpowering?

If the GPIO is specified as active-low in the Device Tree, a logical 0
drives the physical pin high. Because this initialization happens during
probe before the device regulator is turned on in spi_hid_of_power_up(),
a high voltage might be driven into an unpowered peripheral. This could
forward-bias ESD diodes and backpower the device.

> +	if (IS_ERR(conf->reset_gpio)) {
> +		dev_err(dev, "%s: error getting reset GPIO.", __func__);

[Severity: Low]
Does this code unnecessarily log errors during normal boot sequencing?

If devm_gpiod_get() returns -EPROBE_DEFER, the error is unconditionally
logged by dev_err() before being returned.

> +		return PTR_ERR(conf->reset_gpio);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int spi_hid_of_assert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_of_config *conf = container_of(ops,
> +						      struct spi_hid_of_config,
> +						      ops);
> +
> +	gpiod_set_value(conf->reset_gpio, 1);

[Severity: Medium]
Should this use gpiod_set_value_cansleep() instead of gpiod_set_value()?

The assert_reset callback is invoked from sleepable contexts like probe
and workqueues. If the reset GPIO is attached to a sleepable controller
such as an I2C or SPI expander, calling the non-sleeping gpiod_set_value()
will trigger a WARN_ON in the GPIO core.

> +	return 0;
> +}
> +
> +static int spi_hid_of_deassert_reset(struct spihid_ops *ops)
> +{
> +	struct spi_hid_of_config *conf = container_of(ops,
> +						      struct spi_hid_of_config,
> +						      ops);
> +
> +	gpiod_set_value(conf->reset_gpio, 0);

[Severity: Medium]
Similarly, should this use gpiod_set_value_cansleep() since deassert_reset
can also be called from sleepable contexts?

> +	return 0;
> +}

[ ... ]

> +static int spi_hid_of_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct spi_hid_of_config *config;
> +	int error;

[ ... ]

> +	error = spi_hid_of_populate_config(config, dev);
> +	if (error) {
> +		dev_err(dev, "%s: unable to populate config data.", __func__);

[Severity: Low]
Does this code unnecessarily log a second error on probe deferral?

If spi_hid_of_populate_config() returns -EPROBE_DEFER, dev_err()
will unconditionally log it here as well.

> +		return error;
> +	}
> +
> +	return spi_hid_core_probe(spi, &config->ops, &config->property_conf);
> +}

[ ... ]

In relation to the core code enabled by this transport driver:

[Severity: High]
Does the core input handling allow a heap buffer overflow via an
attacker-controlled report_length?

In drivers/hid/spi-hid/spi-hid-core.c:spi_hid_dev_irq(), the input report
header is read, and header.report_length is checked:

	if (shid->desc.max_input_length != 0 &&
	    header->report_length > shid->desc.max_input_length) {

The max_input_length is completely controlled by the SPI device and can
be set to 0 to bypass the check entirely, or set to a large value.

The driver then initiates a DMA read:

		error = spi_hid_input_sync(shid, shid->input->body, header.report_length,
					   false);

Because the actual allocated buffer size (shid->bufsize) is determined
independently by the HID report descriptors, a malicious device could
send small report descriptors but huge report_length values, causing a
DMA read directly past the bounds of the shid->input->body heap buffer.

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