RE: [PATCH rtw v4 1/4] wifi: rtw89: fix use-after-free in rtw89_core_tx_kick_off_and_wait()

[Ping-Ke Shih <pkshih@realtek.com>]

Zong-Zhe Yang <kevin_yang@realtek.com> wrote:
> Ping-Ke Shih <pkshih@realtek.com> wrote:
> >
> > Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> >
> > [...]
> >
> > > @@ -6181,6 +6187,27 @@ rtw89_assoc_link_rcu_dereference(struct rtw89_dev *rtwdev,
> > u8 macid)
> > >         list_first_entry_or_null(&p->dlink_pool,
> > > typeof(*p->links_inst), dlink_schd); \
> > >  })
> > >
> > > +static inline void rtw89_tx_wait_release(struct rtw89_tx_wait_info
> > > +*wait) {
> > > +       dev_kfree_skb_any(wait->skb);
> > > +       kfree_rcu(wait, rcu_head);
> > > +}
> > > +
> > > +static inline void rtw89_tx_wait_list_clear(struct rtw89_dev *rtwdev)
> > > +{
> > > +       struct rtw89_tx_wait_info *wait, *tmp;
> > > +
> > > +       lockdep_assert_wiphy(rtwdev->hw->wiphy);
> > > +
> > > +       list_for_each_entry_safe(wait, tmp, &rtwdev->tx_waits, list) {
> > > +               if (!wait_for_completion_timeout(&wait->completion,
> > > +
> > RTW89_TX_WAIT_DEFAULT_TIMEOUT))
> > > +                       continue;
> >
> >
> > Why should we wait 10ms? Just try_wait_for_completion()?
> >
> > Since TX completion might be missing (rtw89_core_stop(), for example), shouldn't we
> > unconditionally free all in wait list for that case?
> >
> 
> In hci reset (when we release pending skb), the condition will become true.
> So, all left will be freed at that time. Before that, there is no logic to ensure no
> more completing side, so it cannot be unconditionally freed unless we don't
> want to double check if those, which timed out, are done at some moment.
> 
> (e.g. core stop will do hci reset)

Thanks for the explanation. 

Just consider try_wait_for_completion() then.

By the way, if want a delay for timeout case, use delayed work for tx_wait_work
instead.

> 
> >
> > > +               list_del(&wait->list);
> > > +               rtw89_tx_wait_release(wait);
> > > +       }
> > > +}
> > > +
> > >  static inline int rtw89_hci_tx_write(struct rtw89_dev *rtwdev,
> > >                                      struct rtw89_core_tx_request *tx_req)
> > >  {
> >