lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <f70554b9-eedf-43e4-9a55-a809e9b9e89a@buaa.edu.cn> Date: Tue, 4 Apr 2023 17:58:13 +0800 From: Jia-Ju Bai <baijiaju@...a.edu.cn> To: Simon Horman <simon.horman@...igine.com> Cc: johannes@...solutions.net, davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, linux-wireless@...r.kernel.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org Subject: Re: [PATCH v2] net: mac80211: Add NULL checks for sta->sdata Hi Simon, Thanks for your reply, and sorry for the delay. On 2023/3/26 23:05, Simon Horman wrote: > 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@...a.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. I checked the source code again (including the latest version 6.3-rc5). The first line of the body of ___ieee80211_stop_rx_ba_session() is: struct ieee80211_local *local = sta->local; Thus, there is no dereference of sta->sdata. In a different function, namely ___ieee80211_start_rx_ba_session(), the first line is: struct ieee80211_local *local = sta->sdata->local; Thus, there should be a NULL check for sta->sdata in this function. I will add this in my patch v3. > > >> >> - 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? Okay, I will use WARN_ON in my patch v3. > >> + >> + 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. Okay, I will use WARN_ON in my patch v3. > >> ht_dbg(sdata, "Stopping Tx BA session for %pM tid %d\n", >> sta->sta.addr, tid); >> Best wishes, Jia-Ju Bai
Powered by blists - more mailing lists