linux-kernel - Re: [PATCH v2 1/4] spi: spi-geni-qcom: Fix geni_spi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <160817646614.1580929.10863822116174167513@swboyd.mtv.corp.google.com>
Date:   Wed, 16 Dec 2020 19:41:06 -0800
From:   Stephen Boyd <swboyd@...omium.org>
To:     Douglas Anderson <dianders@...omium.org>,
        Mark Brown <broonie@...nel.org>
Cc:     msavaliy@....qualcomm.com, akashast@...eaurora.org,
        Roja Rani Yarubandi <rojay@...eaurora.org>,
        Douglas Anderson <dianders@...omium.org>,
        Alok Chauhan <alokc@...eaurora.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Dilip Kota <dkota@...eaurora.org>,
        Girish Mahadevan <girishm@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-spi@...r.kernel.org
Subject: Re: [PATCH v2 1/4] spi: spi-geni-qcom: Fix geni_spi_isr() NULL dereference in timeout case

Quoting Douglas Anderson (2020-12-16 14:41:49)
> In commit 7ba9bdcb91f6 ("spi: spi-geni-qcom: Don't keep a local state
> variable") we changed handle_fifo_timeout() so that we set
> "mas->cur_xfer" to NULL to make absolutely sure that we don't mess
> with the buffers from the previous transfer in the timeout case.
> 
> Unfortunately, this caused the IRQ handler to dereference NULL in some
> cases.  One case:
> 
>   CPU0                           CPU1
>   ----                           ----
>                                  setup_fifo_xfer()
>                                   geni_se_setup_m_cmd()
>                                  <hardware starts transfer>
>                                  <transfer completes in hardware>
>                                  <hardware sets M_RX_FIFO_WATERMARK_EN in m_irq>
>                                  ...
>                                  handle_fifo_timeout()
>                                   spin_lock_irq(mas->lock)
>                                   mas->cur_xfer = NULL
>                                   geni_se_cancel_m_cmd()
>                                   spin_unlock_irq(mas->lock)
> 
>   geni_spi_isr()
>    spin_lock(mas->lock)
>    if (m_irq & M_RX_FIFO_WATERMARK_EN)
>     geni_spi_handle_rx()
>      mas->cur_xfer NULL dereference!
> 
> tl;dr: Seriously delayed interrupts for RX/TX can lead to timeout
> handling setting mas->cur_xfer to NULL.
> 
> Let's check for the NULL transfer in the TX and RX cases and reset the
> watermark or clear out the fifo respectively to put the hardware back
> into a sane state.
> 
> NOTE: things still could get confused if we get timeouts all the way
> through handle_fifo_timeout() and then start a new transfer because
> interrupts from the old transfer / cancel / abort could still be
> pending.  A future patch will help this corner case.
> 
> Fixes: 561de45f72bd ("spi: spi-geni-qcom: Add SPI driver support for GENI based QUP")
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---

Minor nits below.

Reviewed-by: Stephen Boyd <swboyd@...omium.org>

> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 25810a7eef10..bf55abbd39f1 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -354,6 +354,12 @@ static bool geni_spi_handle_tx(struct spi_geni_master *mas)
>         unsigned int bytes_per_fifo_word = geni_byte_per_fifo_word(mas);
>         unsigned int i = 0;
>  
> +       /* Stop the watermark IRQ if nothing to send */
> +       if (!mas->cur_xfer) {
> +               writel(0, se->base + SE_GENI_TX_WATERMARK_REG);
> +               return false;
> +       }
> +
>         max_bytes = (mas->tx_fifo_depth - mas->tx_wm) * bytes_per_fifo_word;
>         if (mas->tx_rem_bytes < max_bytes)
>                 max_bytes = mas->tx_rem_bytes;
> @@ -396,6 +402,14 @@ static void geni_spi_handle_rx(struct spi_geni_master *mas)
>                 if (rx_last_byte_valid && rx_last_byte_valid < 4)
>                         rx_bytes -= bytes_per_fifo_word - rx_last_byte_valid;
>         }
> +
> +       /* Clear out the FIFO and bail if nowhere to put it */
> +       if (mas->cur_xfer == NULL) {

This is still == NULL though. :(

> +               while (i++ < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word))

A for loop is also fine, but I think it would push the 80 character
limit which is probably OK. I'm happy either way, but a for loop is
usually easier to reason about. I suggested while to fit within 80
characters :/

		for (i = 0; i < DIV_ROUND_UP(rx_bytes, bytes_per_fifo_word); i++)

> +                       readl(se->base + SE_GENI_RX_FIFOn);
> +               return;
> +       }