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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] hid: sleeping in atomic context via synchronous output_report
- [High] spi: dma: unaligned DMA transfer buffers cause memory corruption
- [High] hid: spi-hid: use-after-free of transport buffers on concurrent realloc
- [Medium] hid: spi-hid: SPI_HID_REFRESH_IN_PROGRESS breaks hid_device_io_start() during probe
--

commit 4647f83267b811c038d6b541aee03ea186664da4
Author: Jingyuan Liang <jingyliang@chromium.org>
Subject: HID: spi-hid: add HID SPI protocol implementation

This commit adds a new driver for the HID over SPI protocol. It supports
basic input and output reports but omits features like multi-fragment
reports and power states.

> diff --git a/drivers/hid/spi-hid/spi-hid-core.c b/drivers/hid/spi-hid/spi-hid-core.c
> --- a/drivers/hid/spi-hid/spi-hid-core.c
> +++ b/drivers/hid/spi-hid/spi-hid-core.c
[ ... ]
> +static int spi_hid_input_sync(struct spi_hid *shid, void *buf, u16 length,
> +			      bool is_header)
> +{
> +	int error;
> +
> +	shid->input_transfer[0].tx_buf = is_header ?
> +					 shid->read_approval_header :
> +					 shid->read_approval_body;
> +	shid->input_transfer[0].len = SPI_HID_READ_APPROVAL_LEN;
> +
> +	shid->input_transfer[1].rx_buf = buf;

[Severity: High]
Could this cause memory corruption on non-cache-coherent architectures?

The read_approval_body is a 5-byte array that immediately follows
read_approval_header in struct spi_hid. Since it starts at an unaligned
5-byte offset, passing it as tx_buf to the DMA sync path violates
architecture DMA API rules.

The rx_buf parameter also receives unaligned memory when buf is passed as
shid->input->body later in the IRQ handler, which starts 4 bytes after the
dynamically allocated input header. This can trigger CONFIG_DMA_API_DEBUG
warnings or corrupt adjacent memory.

[ ... ]
> +static int __spi_hid_send_output_report(struct spi_hid *shid,
> +					struct spi_hid_output_report *report,
> +					u8 expected_response)
> +{
> +	struct spi_hid_output_buf *buf = shid->output;
> +	struct device *dev = &shid->spi->dev;
> +	u16 report_length;
> +	u16 padded_length;
> +	u8 padding;
> +	int error;
> +
> +	lockdep_assert_held(&shid->output_lock);

[Severity: High]
Is there a potential use-after-free here if the output buffer is reallocated
concurrently?

When the HID core calls spi_hid_ll_start() dynamically from userspace or
BPF, it expands the transport buffers by calling spi_hid_alloc_buffers()
if the report sizes have increased. This reallocation uses devm_krealloc()
to free shid->output and replace it.

However, spi_hid_ll_start() does not appear to acquire shid->output_lock
before reallocating. If a spontaneous device reset interrupt schedules
reset_work on a global worker thread:

spi_hid_reset_work()
  spi_hid_reset_response()
    spi_hid_sync_request()
      __spi_hid_send_output_report()

This path can execute concurrently, accessing the freed shid->output buffer
under the lock, leading to a use-after-free when writing the header and
payload.

[ ... ]
> +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, 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;
> +
> +	error = __spi_hid_send_output_report(shid, report,
> +					     spi_hid_response_type(report->report_type));

[Severity: High]
Does this sleeping lock violation trigger scheduling while atomic warnings?

According to the HID subsystem guidelines, the low-level output_report
callback is used for high-throughput data on the interrupt channel and
must be asynchronous.

When spi_hid_ll_output_report() calls spi_hid_send_output_report() or
spi_hid_sync_request(), these helpers immediately acquire shid->output_lock
(a sleeping mutex) and perform a synchronous, blocking SPI transfer. Since
the callback can be invoked from atomic contexts (like updating LED states
from an input event handler), sleeping here may cause panics.

[ ... ]
> +static int spi_hid_input_report_handler(struct spi_hid *shid,
> +					struct spi_hid_input_buf *buf)
> +{
> +	struct device *dev = &shid->spi->dev;
> +	struct hid_device *hid;
> +	struct spi_hid_input_report r;
> +	int error = 0;
> +
> +	scoped_guard(mutex, &shid->io_lock) {
> +		if (!test_bit(SPI_HID_READY, &shid->flags) ||
> +		    !test_bit(SPI_HID_STARTED, &shid->flags) ||
> +		    test_bit(SPI_HID_REFRESH_IN_PROGRESS, &shid->flags) || !shid->hid) {
> +			dev_dbg(dev, "HID not ready (flags 0x%lx), dropping input report\n",
> +				shid->flags);
> +			return 0;
> +		}

[Severity: Medium]
Will this prevent drivers from receiving input reports during probe?

When spi_hid_refresh_device() is executed, it sets the
SPI_HID_REFRESH_IN_PROGRESS flag before tearing down the old HID device
and creating the new one:

spi_hid_refresh_device()
  set_bit(SPI_HID_REFRESH_IN_PROGRESS, ...)
  spi_hid_create_device()
    hid_add_device()
      -> driver probe()

During this entire time, the input report handler will drop all incoming
reports. This conflicts with the HID core API contract, which explicitly
allows a driver to receive inputs during its probe phase by calling
hid_device_io_start(). A driver that requires a firmware handshake or
data during probe could hang or fail upon device refresh.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-send-upstream-v5-0-384af01da3ee@chromium.org?part=5