Messages in this thread | | | Date | Sun, 26 Mar 2023 17:05:02 +0200 | From | Simon Horman <> | Subject | Re: [PATCH v2] net: mac80211: Add NULL checks for sta->sdata |
| |
On Tue, Mar 21, 2023 at 05:31:22PM +0800, Jia-Ju Bai wrote: > In a previous commit 69403bad97aa ("wifi: mac80211: sdata can be NULL > during AMPDU start"), sta->sdata can be NULL, and thus it should be > checked before being used. > > However, in the same call stack, sta->sdata is also used in the > following functions: > > ieee80211_ba_session_work() > ___ieee80211_stop_rx_ba_session(sta) > ht_dbg(sta->sdata, ...); -> No check > sdata_info(sta->sdata, ...); -> No check > ieee80211_send_delba(sta->sdata, ...) -> No check > ___ieee80211_start_rx_ba_session(sta) > ht_dbg(sta->sdata, ...); -> No check > ht_dbg_ratelimited(sta->sdata, ...); -> No check > ieee80211_tx_ba_session_handle_start(sta) > sdata = sta->sdata; if (!sdata) -> Add check by previous commit > ___ieee80211_stop_tx_ba_session(sdata) > ht_dbg(sta->sdata, ...); -> No check > ieee80211_start_tx_ba_cb(sdata) > sdata = sta->sdata; local = sdata->local -> No check > ieee80211_stop_tx_ba_cb(sdata) > ht_dbg(sta->sdata, ...); -> No check > > Thus, to avoid possible null-pointer dereferences, the related checks > should be added. > > These bugs are reported by a static analysis tool implemented by myself, > and they are found by extending a known bug fixed in the previous commit. > Thus, they could be theoretical bugs. > > Signed-off-by: Jia-Ju Bai <baijiaju@buaa.edu.cn> > --- > v2: > * Fix an error reported by checkpatch.pl, and make the bug finding > process more clear in the description. Thanks for Simon's advice. > --- > net/mac80211/agg-rx.c | 68 ++++++++++++++++++++++++++----------------- > net/mac80211/agg-tx.c | 16 ++++++++-- > 2 files changed, 55 insertions(+), 29 deletions(-) > > diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c > index c6fa53230450..6616970785a2 100644 > --- a/net/mac80211/agg-rx.c > +++ b/net/mac80211/agg-rx.c > @@ -80,19 +80,21 @@ void ___ieee80211_stop_rx_ba_session(struct sta_info *sta, u16 tid, > RCU_INIT_POINTER(sta->ampdu_mlme.tid_rx[tid], NULL); > __clear_bit(tid, sta->ampdu_mlme.agg_session_valid); > > - ht_dbg(sta->sdata, > - "Rx BA session stop requested for %pM tid %u %s reason: %d\n", > - sta->sta.addr, tid, > - initiator == WLAN_BACK_RECIPIENT ? "recipient" : "initiator", > - (int)reason); > + if (sta->sdata) { > + ht_dbg(sta->sdata, > + "Rx BA session stop requested for %pM tid %u %s reason: %d\n", > + sta->sta.addr, tid, > + initiator == WLAN_BACK_RECIPIENT ? "recipient" : "initiator", > + (int)reason); > + }
The first line of the body of ___ieee80211_stop_rx_ba_session() is:
struct ieee80211_local *local = sta->sdata->local;
So a NULL pointer dereference will have occurred before the checks this change adds to that function.
> > - if (drv_ampdu_action(local, sta->sdata, ¶ms)) > + if (sta->sdata && drv_ampdu_action(local, sta->sdata, ¶ms)) > sdata_info(sta->sdata, > "HW problem - can not stop rx aggregation for %pM tid %d\n", > sta->sta.addr, tid); >
...
> diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c > index f9514bacbd4a..03b31b6e7ac7 100644 > --- a/net/mac80211/agg-tx.c > +++ b/net/mac80211/agg-tx.c > @@ -368,8 +368,10 @@ int ___ieee80211_stop_tx_ba_session(struct sta_info *sta, u16 tid, > > spin_unlock_bh(&sta->lock); > > - ht_dbg(sta->sdata, "Tx BA session stop requested for %pM tid %u\n", > - sta->sta.addr, tid); > + if (sta->sdata) { > + ht_dbg(sta->sdata, "Tx BA session stop requested for %pM tid %u\n", > + sta->sta.addr, tid); > + }
This seems clean :)
> del_timer_sync(&tid_tx->addba_resp_timer); > del_timer_sync(&tid_tx->session_timer); > @@ -776,7 +778,12 @@ void ieee80211_start_tx_ba_cb(struct sta_info *sta, int tid, > struct tid_ampdu_tx *tid_tx) > { > struct ieee80211_sub_if_data *sdata = sta->sdata; > - struct ieee80211_local *local = sdata->local; > + struct ieee80211_local *local; > + > + if (!sdata) > + return;
I'm not sure that silently ignoring non-existent sdata is the right approach. Perhaps a WARN_ON or WARN_ONCE is appropriate?
> + > + local = sdata->local; > > if (WARN_ON(test_and_set_bit(HT_AGG_STATE_DRV_READY, &tid_tx->state))) > return; > @@ -902,6 +909,9 @@ void ieee80211_stop_tx_ba_cb(struct sta_info *sta, int tid, > bool send_delba = false; > bool start_txq = false; > > + if (!sdata) > + return; > +
Ditto.
> ht_dbg(sdata, "Stopping Tx BA session for %pM tid %d\n", > sta->sta.addr, tid); >
| |