Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues: - [Critical] A valid SPI message containing a transfer with length > 0 but both `tx_buf` and `rx_buf` set to NULL will cause a kernel crash when the driver dereferences the NULL buffer during FIFO writes. - [High] Early error returns in `stm32_ospi_transfer_one_message` fail to finalize the SPI message, resulting in a permanent hang for callers. -- commit 77e013496da647c31add0f5085a07d31c7c250b2 Author: Kaitao Cheng <[email protected]> spi: stm32-ospi: Open-code message transfer walk This commit modifies stm32_ospi_transfer_one_message() to open-code the list iteration over message transfers. It prepares the loop to observe cursor updates explicitly, anticipating a later list iterator update. > diff --git a/drivers/spi/spi-stm32-ospi.c b/drivers/spi/spi-stm32-ospi.c > index 3757f6ba8fc61..fe92bb06251be 100644 > --- a/drivers/spi/spi-stm32-ospi.c > +++ b/drivers/spi/spi-stm32-ospi.c > @@ -674,7 +674,9 @@ static int stm32_ospi_transfer_one_message(struct > spi_controller *ctrl, [Severity: High] This is a pre-existing issue, but do the early error returns in stm32_ospi_transfer_one_message() fail to finalize the SPI message? If cs_gpiod is null or pm_runtime_resume_and_get() fails, the function returns early: if (!cs_gpiod) return -EOPNOTSUPP; ret = pm_runtime_resume_and_get(ospi->dev); if (ret < 0) return ret; Since these paths bypass spi_finalize_current_message(), does this leave the SPI core waiting indefinitely and cause a deadlock for callers? [Severity: High] This is also a pre-existing issue, but can this driver crash if an SPI message contains a dummy transfer with both tx_buf and rx_buf set to NULL? The SPI core allows dummy transfers with both buffers set to NULL. If a transfer reaches stm32_ospi_transfer_one_message() without the dummy_data flag set, the driver defaults to a write operation and assigns the NULL tx_buf to the operation: } else { ospi->fmode = CR_FMODE_INDW; op.data.buswidth = transfer->tx_nbits; op.data.dir = SPI_MEM_DATA_OUT; op.data.buf.out = transfer->tx_buf; } This eventually leads to stm32_ospi_send() and stm32_ospi_xfer(). Because DMA mapping of a NULL pointer fails, it falls back to PIO mode by calling stm32_ospi_poll(). Inside stm32_ospi_poll(), the stm32_ospi_write_fifo() callback is invoked, which dereferences the NULL pointer directly: switch (len) { case sizeof(u32): writel_relaxed(*((u32 *)val), addr); break; Could this trigger a kernel panic and allow a local denial of service? > > gpiod_set_value_cansleep(cs_gpiod, true); > > - list_for_each_entry(transfer, &msg->transfers, transfer_list) { > + for (transfer = list_first_entry(&msg->transfers, typeof(*transfer), > transfer_list); > + !list_entry_is_head(transfer, &msg->transfers, transfer_list); > + transfer = list_next_entry(transfer, transfer_list)) { > u8 dummy_bytes = 0; > > memset(&op, 0, sizeof(op)); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=8
