Re: [PATCH] mwifiex: Fix use-after-free bug due to race condition between main thread thread and timer thread

[Zheng Hacker]

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

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 after
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@xxxxxxxxx> 于2023年2月24日周五 13:37写道：
>
> 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 after
> 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@xxxxxxxxxxxx> 于2023年2月23日周四 05:20写道：
> >
> > 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