Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr

From: Stephen Boyd <swboyd@chromium.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Roja Rani Yarubandi <rojay@codeaurora.org>,
	Mark Brown <broonie@kernel.org>, Andy Gross <agross@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	linux-spi <linux-spi@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Akash Asthana <akashast@codeaurora.org>,
	msavaliy@qti.qualcomm.com
Subject: Re: [PATCH] spi: spi-geni-qcom: Fix NULL pointer access in geni_spi_isr
Date: Thu, 10 Dec 2020 14:57:57 -0800	[thread overview]
Message-ID: <160764107797.1580929.14768824290834396298@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <CAD=FV=WtU3cnRe6pDKFMA9_0cnQFtSOyohY_bJwZObK+KrbhVQ@mail.gmail.com>

Quoting Doug Anderson (2020-12-10 09:14:15)
> 
> This is my untested belief of what's happening
> 
>  CPU0                                CPU1
>  ----                                ----
>                                      setup_fifo_xfer()
>                                       ...
>                                       geni_se_setup_m_cmd()
>                                       <hardware starts transfer>
>  <unrelated interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         <transfer complets in hardware>
>  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         handle_fifo_timeout()
>  <continued interrupt storm>          spin_lock_irq()
>  <continued interrupt storm>          mas->cur_xfer = NULL
>  <continued interrupt storm>          geni_se_cancel_m_cmd()
>  <continued interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>          wait_for_completion_timeout() => timeout
>  <continued interrupt storm>          spin_lock_irq()
>  <continued interrupt storm>          geni_se_abort_m_cmd()
>  <continued interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>          wait_for_completion_timeout() => timeout
>  <interrupt storm ends>
>  geni_spi_isr()
>   spin_lock()
>   if (m_irq & M_RX_FIFO_WATERMARK_EN)
>    geni_spi_handle_rx()
>     mas->cur_xfer NULL derefrence

Ok so the one line summary is "If geni_spi_isr() is sufficiently delayed
then we may deref NULL in the irq handler because the handler tries to
deref mas->cur_xfer after the timeout handling code has set it to NULL".

  CPU0                                CPU1
  ----                                ----
                                      setup_fifo_xfer()
                                       ...
                                       geni_se_setup_m_cmd()
                                       <hardware starts transfer>
  unrelated_irq_handler()              spin_unlock_irq()
   ...                                
                                        <transfer completes in hardware>
                                        <hardware sets M_RX_FIFO_WATERMARK_EN>

                                       handle_fifo_timeout()
                                        spin_lock_irq()
                                        mas->cur_xfer = NULL
                                        geni_se_cancel_m_cmd()
                                        spin_unlock_irq()
                                        wait_for_completion_timeout() => timeout
                                        spin_lock_irq()
                                        geni_se_abort_m_cmd()
                                        spin_unlock_irq()
                                        wait_for_completion_timeout() => timeout
   return IRQ_HANDLED;
  gic_handle_irq()
   geni_spi_isr()
    spin_lock()
    if (m_irq & M_RX_FIFO_WATERMARK_EN)
     geni_spi_handle_rx()
      rx_buf = mas->cur_xfer->rx_buf <--- OOPS!

> With my proposed fix, I believe that would transform into:
> 
>  CPU0                                CPU1
>  ----                                ----
>                                      setup_fifo_xfer()
>                                       ...
>                                       geni_se_setup_m_cmd()
>                                       <hardware starts transfer>
>  <unrelated interrupt storm>          spin_unlock_irq()
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         <transfer complets in hardware>
>  <continued interrupt storm>         <hardware sets M_RX_FIFO_WATERMARK_EN>
>  <continued interrupt storm>         <time passes>
>  <continued interrupt storm>         handle_fifo_timeout()
>  <continued interrupt storm>          synchronize_irq()
>  <continued interrupt storm>           <time passes>
>  <interrupt storm ends>
>  geni_spi_isr()
>   ...
>                                        <synchronize_irq() finishes>
>                                       spin_lock_irq()
>                                       mas->cur_xfer = NULL
>                                       geni_se_cancel_m_cmd()
>                                       spin_unlock_irq()
>  geni_spi_isr()
>    ...
>                                       wait_for_completion_timeout() => success
> 
> The extra synchronize_irq() I was suggesting at the end of the
> function would be an extra bit of paranoia.  Maybe a new storm showed
> up while we were processing the timeout?

Shouldn't we check in the timeout logic to see if m_irq has
M_RX_FIFO_WATERMARK_EN or M_TX_FIFO_WATERMARK_EN set instead? Similarly
for the CS assert/deassert stuff. If the timeout hits but one of those
bits are set then it seems we've run into some poor irqsoff section but
the hardware is still working. Calling synchronize_irq() wouldn't help
if the CPU handling the irqs (i.e. CPU0) had irqs off for a long time,
right? It will only ensure that other irq handlers have completed, which
may be a problem, but not the only one.

TL;DR: Peek at the irq status register in the timeout logic and skip it
if the irq is pending?