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> <CAJedcCykky7E_uyeU=Pj1HR0rcpUTF1tKJ-2UmmM33bweDg=yw@mail.gmail.com>
In-Reply-To: <CAJedcCykky7E_uyeU=Pj1HR0rcpUTF1tKJ-2UmmM33bweDg=yw@mail.gmail.com>
From:   Zheng Hacker <hackerzheng666@gmail.com>
Date:   Fri, 24 Feb 2023 14:17:59 +0800
Message-ID: <CAJedcCztEkE=EB2GmH=BpTvD=r_bwGXk3RYDM2FU=f_SvEaJHA@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

This email is broken for the statement is too long, Here is the newest emai=
l.

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


              retn 0 in mwifiex_dnld_cmd_to_fw
              retn 0 in mwifiex_exec_next_cmd
              retn 0 in  mwifiex_main_process
              retn -EINPROGRESS in mwifiex_init_fw
              mwifiex_free_adapter when in error
              // free adapter
              complete_all(fw_done);


                                              [*]Go on
                                              Use adapter->fw_done

Best regards,
Zheng


Zheng Hacker <hackerzheng666@gmail.com> =E4=BA=8E2023=E5=B9=B42=E6=9C=8824=
=E6=97=A5=E5=91=A8=E4=BA=94 13:37=E5=86=99=E9=81=93=EF=BC=9A
>
> Hello Brain,
>
> Thanks for your detailed review. Sorry we missed something. As you say, W=
e are
> lacking some consideration, the pointed race path could not happen. But a=
fter
> 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 kno=
w.
>
>         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_timeo=
ut_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=882=
3=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 pointe=
d
> > 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