Re: [PATCH] spi: cadence: recover the controller after a failed transfer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] spi: cadence: unreset speed_hz in cdns_spi_handle_err() runs bus at max clk
- [High] spi: cadence: missing IRQ sync in cdns_spi_handle_err() causes buffer UAF
- [Medium] spi: cadence: unreset CDNS_SPI_THLD in cdns_spi_handle_err() breaks target
--

commit ac57da57c19b61a2d1f323d2cee0fc683cff3b77
Author: Mario Behn <mario.behn@gmail.com>

spi: cadence: recover the controller after a failed transfer

This commit adds a handle_err callback to the Cadence spi controller driver
to mask interrupts, drain stale bytes from the receive fifo, and reinitialize
the controller hardware after a transfer times out or fails.

> diff --git a/drivers/spi/spi-cadence.c b/drivers/spi/spi-cadence.c
> index 50ef3a7871c22..474ce14a59b3e 100644
> --- a/drivers/spi/spi-cadence.c
> +++ b/drivers/spi/spi-cadence.c
> @@ -542,6 +542,30 @@ static int cdns_transfer_one(struct spi_controller *ctlr,
>  	return transfer->len;
>  }
>  
> +/**
> + * cdns_spi_handle_err - Recover the controller after a failed transfer
> + * @ctlr:	Pointer to the spi_controller structure
> + * @msg:	Pointer to the spi_message that failed
> + *
> + * A transfer that timed out leaves the bytes it clocked in sitting in the
> + * RX FIFO, because the interrupt that reads them did not run in time and
> + * the host path never drains the FIFO between transfers.  Every later
> + * transfer would then read those stale bytes first and return shifted
> + * data.  Mask the interrupts, drop the stale bytes and bring the
> + * controller back to its initial state; the next message sets the clock
> + * and the chip select again on its way through transfer_one.
> + */
> +static void cdns_spi_handle_err(struct spi_controller *ctlr,
> +				struct spi_message *msg)
> +{
> +	struct cdns_spi *xspi = spi_controller_get_devdata(ctlr);
> +
> +	cdns_spi_write(xspi, CDNS_SPI_IDR, CDNS_SPI_IXR_ALL);
> +	xspi->tx_bytes = 0;
> +	xspi->rx_bytes = 0;

[Severity: High]
Can cdns_spi_handle_err() race with an in-flight or already-woken
cdns_spi_irq() handler when a transfer times out in spi_transfer_wait()?

In drivers/spi/spi-cadence.c, cdns_spi_probe() registers cdns_spi_irq() via
devm_request_irq() with flags set to 0, which runs as a forced threaded
handler on preempt-rt or concurrently on another processor. Writing to
CDNS_SPI_IDR masks peripheral interrupt generation, but cdns_spi_handle_err()
does not synchronize with the irq handler, nor does it clear xspi->rxbuf or
xspi->txbuf before spi_transfer_one_message() finalizes the message and the
caller frees transfer->rx_buf and transfer->tx_buf.

If cdns_spi_irq() is already running—for example, across the udelay(10) call
or inside cdns_spi_process_fifo() with local ntx and nrx counts—could it read
or write freed transfer buffers via cdns_spi_reader() and cdns_spi_writer(),
or race with cdns_spi_init_hw() while accessing hardware fifos?

In addition, cdns_spi_irq() reads raw status bits without masking against
CDNS_SPI_IMR:

drivers/spi/spi-cadence.c:cdns_spi_irq() {
	intr_status = cdns_spi_read(xspi, CDNS_SPI_ISR);
	cdns_spi_write(xspi, CDNS_SPI_ISR, intr_status);
	...
	} else if (intr_status & CDNS_SPI_IXR_TXOW) {
		...
		if (xspi->tx_bytes) {
			cdns_spi_process_fifo(ctlr, xspi, trans_cnt, trans_cnt);
		} else {
			udelay(10);
			cdns_spi_process_fifo(ctlr, xspi, 0, trans_cnt);
			cdns_spi_write(xspi, CDNS_SPI_IDR,
				       CDNS_SPI_IXR_DEFAULT);
			spi_finalize_current_transfer(ctlr);
		}
}

Because cdns_spi_init_hw() re-enables the controller (CDNS_SPI_ER_ENABLE)
with an empty transmit fifo, CDNS_SPI_IXR_TXOW becomes set in CDNS_SPI_ISR.
If a delayed irq thread runs after cdns_spi_handle_err() finishes, wouldn't
it see CDNS_SPI_IXR_TXOW set and xspi->tx_bytes == 0, disable interrupts, and
call spi_finalize_current_transfer() prematurely on the next transfer?

> +	cdns_spi_init_hw(xspi, spi_controller_is_target(ctlr));

[Severity: High]
Does calling cdns_spi_init_hw() in drivers/spi/spi-cadence.c without
resetting xspi->speed_hz leave the bus running at the maximum clock rate on
subsequent transfers?

In host mode, cdns_spi_init_hw() writes CDNS_SPI_CR_DEFAULT to CDNS_SPI_CR,
which resets the hardware baud rate divisor bits (CDNS_SPI_CR_BAUD_DIV) back
to CDNS_SPI_CR_BAUD_DIV_4 (xspi->clk_rate / 4, i.e., ctlr->max_speed_hz):

drivers/spi/spi-cadence.c:cdns_spi_init_hw() {
	if (!is_target)
		ctrl_reg |= CDNS_SPI_CR_DEFAULT;
	...
	cdns_spi_write(xspi, CDNS_SPI_CR, ctrl_reg);
}

When the next transfer runs cdns_transfer_one() -> cdns_spi_setup_transfer()
-> cdns_spi_config_clock_freq(), it only reprograms the divisor if
xspi->speed_hz differs from transfer->speed_hz:

drivers/spi/spi-cadence.c:cdns_spi_config_clock_freq() {
	ctrl_reg = cdns_spi_read(xspi, CDNS_SPI_CR);

	/* Set the clock frequency */
	if (xspi->speed_hz != transfer->speed_hz) {
		...
		ctrl_reg &= ~CDNS_SPI_CR_BAUD_DIV;
		ctrl_reg |= baud_rate_val << CDNS_SPI_BAUD_DIV_SHIFT;

		xspi->speed_hz = frequency / (2 << baud_rate_val);
	}
	cdns_spi_write(xspi, CDNS_SPI_CR, ctrl_reg);
}

If the next transfer requests the same frequency that was cached in
xspi->speed_hz before the error, won't cdns_spi_config_clock_freq() skip
updating CDNS_SPI_CR_BAUD_DIV and leave the hardware running at clk_rate / 4
instead of the requested speed?

[Severity: Medium]
What happens in target mode if a transfer with xspi->tx_bytes >
xspi->tx_fifo_depth fails while another message is already queued on the
controller?

In drivers/spi/spi-cadence.c, cdns_transfer_one() raises CDNS_SPI_THLD for
large target transfers:

drivers/spi/spi-cadence.c:cdns_transfer_one() {
	if (!spi_controller_is_target(ctlr)) {
		cdns_spi_setup_transfer(spi, transfer);
	} else {
		if (xspi->tx_bytes > xspi->tx_fifo_depth)
			cdns_spi_write(xspi, CDNS_SPI_THLD, xspi->tx_fifo_depth >> 1);
	}
}

Normally CDNS_SPI_THLD is reset to 0x1 either in cdns_spi_irq() when
xspi->tx_bytes drops below half the fifo depth or in
cdns_unprepare_transfer_hardware() when the queue becomes idle. Neither
cdns_spi_handle_err() nor cdns_spi_init_hw() resets CDNS_SPI_THLD to 0x1.

If a large target transfer is interrupted in spi_transfer_wait() and another
message is queued behind it, __spi_pump_messages() starts the next message
without calling cdns_unprepare_transfer_hardware(). If the next transfer has
tx_bytes <= tx_fifo_depth, should cdns_spi_handle_err() or cdns_spi_init_hw()
reset CDNS_SPI_THLD to 0x1 so that CDNS_SPI_IXR_TXOW does not fire
immediately with xspi->tx_bytes == 0 and complete the transfer prematurely?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105203703.979067-1-mario.behn@gmail.com?part=1