Re: [PATCH] spi: amd: fix len==0 underflow and FIFO overflow

[M, Krishnamoorthi]

Hi,


On 10/14/2026 6:09 PM, Weibin Liu wrote:
> [You don't often get email from liuwb@xxxxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> amd_spi_fifo_xfer() unconditionally decrements xfer->len when a tx_buf
> is present. The SPI core does not reject zero-length transfers, so a
> transfer submitted through the SPI_IOC_MESSAGE ioctl with len == 0 and
> a tx_buf makes the u32 len wrap to 0xFFFFFFFF, and the following
> per-byte loop writes far past the FIFO window.
>
> The driver also never checks the cumulative FIFO offset against
> AMD_SPI_FIFO_SIZE. The core splits each transfer down to the
> controller's max transfer size but does not bound the total message
> length, so a message of several transfers whose total exceeds the FIFO
> overflows it with out-of-bounds MMIO writes (tx) and reads (rx).
>
> Skip zero-length transfers and reject any transfer that would run past
> the end of the FIFO window.
>
> Tested with a user-space replica of the fifo_xfer length arithmetic
> (ASAN build): a zero-length transfer with tx_buf makes len-- wrap to
> 0xffffffff, and skipping zero-length transfers avoids the wrap. No AMD
> SPI hardware was available, so the FIFO bound check was verified by
> inspection against AMD_SPI_FIFO_SIZE.
>
> Fixes: bbb336f39efc ("spi: spi-amd: Add AMD SPI controller driver support")
> Cc: stable@xxxxxxxxxxxxxxx # 5.8+
> Reviewed-by: Guanglei Zhu <zhugl3@xxxxxxxxxxxx>
> Signed-off-by: Weibin Liu <liuwb@xxxxxxxxxxxx>
> ---
>   drivers/spi/spi-amd.c | 17 +++++++++++++++++
>   1 file changed, 17 insertions(+)
>
> diff --git a/drivers/spi/spi-amd.c b/drivers/spi/spi-amd.c
> index 71a6e5c475b0..ddf89e5353c5 100644
> --- a/drivers/spi/spi-amd.c
> +++ b/drivers/spi/spi-amd.c
> @@ -322,11 +322,22 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
>
>          list_for_each_entry(xfer, &message->transfers,
>                              transfer_list) {
> +               /* Skip zero-length transfers to avoid len underflow below */
> +               if (!xfer->len)
> +                       continue;
> +
>                  if (xfer->speed_hz)
>                          amd_set_spi_freq(amd_spi, xfer->speed_hz);
>                  else
>                          amd_set_spi_freq(amd_spi, spi->max_speed_hz);
>
> +               /* Reject transfers that would overflow the FIFO window */
> +               if (fifo_pos + xfer->len >
> +                   AMD_SPI_FIFO_BASE + AMD_SPI_FIFO_SIZE) {
> +                       message->status = -EMSGSIZE;
> +                       goto fin_msg;
> +               }
> +
>                  if (xfer->tx_buf) {
>                          buf = (u8 *)xfer->tx_buf;
>                          if (!tx_len) {
> @@ -369,6 +380,12 @@ static inline int amd_spi_fifo_xfer(struct amd_spi *amd_spi,
>
>                  list_for_each_entry(xfer, &message->transfers, transfer_list)
>                          if (xfer->rx_buf) {
> +                               /* Reject transfers that would overflow the FIFO window */
> +                               if (fifo_pos + xfer->len >
> +                                   AMD_SPI_FIFO_BASE + AMD_SPI_FIFO_SIZE) {
> +                                       message->status = -EMSGSIZE;
> +                                       goto fin_msg;
> +                               }
>                                  buf = (u8 *)xfer->rx_buf;
>                                  /* Read data from FIFO to receive buffer */
>                                  for (i = 0; i < xfer->len; i++)
>
> ---

Thanks for the fix. The zero-length skip and the overflow bound are both 
correct, but the FIFO check can be done more simply and, more 
importantly, before the command is issued.

fifo_pos only advances in the tx branch, so this form can't track the 
cumulative rx length in the first loop. That's why you need the second 
copy in the drain loop - but by then amd_spi_execute_opcode() has 
already run, so an oversized message still drives one SPI transaction 
before being rejected.

Please use tx_len + rx_len against AMD_SPI_FIFO_SIZE and check it in the 
first loop only, so the whole message is validated before execute and 
the drain-loop check can be dropped:

if (xfer->tx_buf) {
    ...
    tx_len += xfer->len;
    if (tx_len + rx_len > AMD_SPI_FIFO_SIZE) {
        message->status = -EMSGSIZE;
        goto fin_msg;
    }
    ...
}
...
/* Store no. of bytes to be received from FIFO */
if (xfer->rx_buf) {
    rx_len += xfer->len;
    if (tx_len + rx_len > AMD_SPI_FIFO_SIZE) {
        message->status = -EMSGSIZE;
        goto fin_msg;
    }
}


Thanks,
Krishna
> base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
> change-id: 20261014-spi-amd-fifo-fix-37ac4ef05bf6
>
> Best regards,
> --
> Weibin Liu <liuwb@xxxxxxxxxxxx>