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. [ ... ] > +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/[email protected]?part=5
