On Fri, Oct 9, 2026 at 3:42 PM <[email protected]> wrote:
>
> 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 <[email protected]>
> 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.
I don't think so. The read approvals are TX-only and never change
after probe, and the RX body only shares a cacheline with the input
header, which isn't touched during the body transfer.
>
> [ ... ]
> > +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.
Thank you. I will fix it in v6.
>
> [ ... ]
> > +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.
No. hid-input defers LED updates from input events to a workqueue, so
->output_report() isn't called from atomic context there, and USB-HID
and I2C-HID already sleep in this callback too.
>
> [ ... ]
> > +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.
Thank you. I will fix it in v6.
>
> --
> Sashiko AI review ·
> https://sashiko.dev/#/patchset/[email protected]?part=5
