MIME-Version: 1.0
References: <20230218075956.1563118-1-zyytlz.wz@163.com> <Y/U+w7aMc+BttZwl@google.com>
 <CAJedcCzmnZCR=XF+zKHiJ+8PNK88sXFDm5n=RnwcTnJfO0ihOw@mail.gmail.com> <Y/aHHSkUOsOsU+Kq@google.com>
In-Reply-To: <Y/aHHSkUOsOsU+Kq@google.com>
From:   Zheng Hacker <hackerzheng666@gmail.com>
Date:   Fri, 24 Feb 2023 13:37:26 +0800
Message-ID: <CAJedcCykky7E_uyeU=Pj1HR0rcpUTF1tKJ-2UmmM33bweDg=yw@mail.gmail.com>
Subject: Re: [PATCH] mwifiex: Fix use-after-free bug due to race condition
 between main thread thread and timer thread
To:     Brian Norris <briannorris@chromium.org>
Cc:     Zheng Wang <zyytlz.wz@163.com>, ganapathi017@gmail.com,
        alex000young@gmail.com, amitkarwar@gmail.com,
        sharvari.harisangam@nxp.com, huxinming820@gmail.com,
        kvalo@kernel.org, davem@davemloft.net, edumazet@google.com,
        kuba@kernel.org, pabeni@redhat.com, linux-wireless@vger.kernel.org,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hello Brain,

Thanks for your detailed review. Sorry we missed something. As you say, We =
are
lacking some consideration, the pointed race path could not happen. But aft=
er
diving deep into the code, we found another path.

Here is the possible path. In summary, if the execution of CPU1 is
stuck by fw_done,
the cpu0 will continue executing and free the adapter gets into
error-path. The cpu1
did not notice that and UAF happened.

If there is something else we didn't know, please feel free to let us know.

        CPU0                                                        CPU1
mwifiex_sdio_probe
mwifiex_add_card
mwifiex_init_hw_fw
request_firmware_nowait
  mwifiex_fw_dpc
    _mwifiex_fw_dpc
      mwifiex_init_fw
        mwifiex_main_process
          mwifiex_exec_next_cmd
            mwifiex_dnld_cmd_to_fw
              mod_timer(&adapter->cmd_timer,..)
                                                        mwifiex_cmd_timeout=
_func

if_ops.card_reset(adapter)

mwifiex_sdio_card_reset
                                                            [*] stuck
in mwifiex_shutdown_sw
              return 0 in mwifiex_dnld_cmd_to_fw
                return 0 in mwifiex_exec_next_cmd
                  return 0 in  mwifiex_main_process
                    return -EINPROGRESS in mwifiex_init_fw
                      get into error path, mwifiex_free_adapter
                        // free adapter
                        complete_all(fw_done);
                                                              [*]Go on
                                                              Use
adapter->fw_done


Best regards,
Zheng


Brian Norris <briannorris@chromium.org> =E4=BA=8E2023=E5=B9=B42=E6=9C=8823=
=E6=97=A5=E5=91=A8=E5=9B=9B 05:20=E5=86=99=E9=81=93=EF=BC=9A
>
> On Wed, Feb 22, 2023 at 12:17:21PM +0800, Zheng Hacker wrote:
> > Could you please provide some advice about the fix?
>
> This entire driver's locking patterns (or lack
> thereof) need rewritten. This driver was probably written by someone
> that doesn't really understand concurrent programming. It really only
> works because the bulk of normal operation is sequentialized into the
> main loop (mwifiex_main_process()). Any time you get outside that,
> you're likely to find bugs.
>
> But now that I've looked a little further, I'm not confident you pointed
> out a real bug. How does mwifiex_sdio_card_reset_work() get past
> mwifiex_shutdown_sw() -> wait_for_completion(adapter->fw_done) ? That
> should ensure that _mwifiex_fw_dpc() is finished, and so we can't hit
> the race you point out.
>
> Note to self: ignore most "static analysis" reports of race conditions,
> unless they have thorough analysis or a runtime reproduction.
>
> Brian