1. Patchew
  2. linux
  3. View series

[PATCH v3] net: mac80211: Add NULL checks for sta->sdata

Jia-Ju Bai posted 1 patch 2 years, 8 months ago 
net/mac80211/agg-rx.c | 95 ++++++++++++++++++++++++++-----------------
net/mac80211/agg-tx.c | 16 ++++++--
2 files changed, 71 insertions(+), 40 deletions(-)
[PATCH v3] net: mac80211: Add NULL checks for sta->sdata
Posted by Jia-Ju Bai 2 years, 8 months ago 
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>
---
v3:
* Add NULL check about sta->sdata related to local in the function
  ___ieee80211_start_rx_ba_session()
---
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 | 95 ++++++++++++++++++++++++++-----------------
 net/mac80211/agg-tx.c | 16 ++++++--
 2 files changed, 71 insertions(+), 40 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index c6fa53230450..f2e9dbd7559a 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);
+	}
 
-	if (drv_ampdu_action(local, sta->sdata, &params))
+	if (sta->sdata && drv_ampdu_action(local, sta->sdata, &params))
 		sdata_info(sta->sdata,
 			   "HW problem - can not stop rx aggregation for %pM tid %d\n",
 			   sta->sta.addr, tid);
 
 	/* check if this is a self generated aggregation halt */
-	if (initiator == WLAN_BACK_RECIPIENT && tx)
+	if (initiator == WLAN_BACK_RECIPIENT && tx && sta->sdata)
 		ieee80211_send_delba(sta->sdata, sta->sta.addr,
 				     tid, WLAN_BACK_RECIPIENT, reason);
 
@@ -256,7 +258,7 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 				      u16 buf_size, bool tx, bool auto_seq,
 				      const struct ieee80211_addba_ext_ie *addbaext)
 {
-	struct ieee80211_local *local = sta->sdata->local;
+	struct ieee80211_local *local = NULL;
 	struct tid_ampdu_rx *tid_agg_rx;
 	struct ieee80211_ampdu_params params = {
 		.sta = &sta->sta,
@@ -270,26 +272,35 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 	u16 status = WLAN_STATUS_REQUEST_DECLINED;
 	u16 max_buf_size;
 
+	if (sta->sdata)
+		local = sta->sdata->local;
+
 	if (tid >= IEEE80211_FIRST_TSPEC_TSID) {
-		ht_dbg(sta->sdata,
-		       "STA %pM requests BA session on unsupported tid %d\n",
-		       sta->sta.addr, tid);
+		if (sta->sdata) {
+			ht_dbg(sta->sdata,
+			       "STA %pM requests BA session on unsupported tid %d\n",
+			       sta->sta.addr, tid);
+		}
 		goto end;
 	}
 
 	if (!sta->sta.deflink.ht_cap.ht_supported &&
 	    !sta->sta.deflink.he_cap.has_he) {
-		ht_dbg(sta->sdata,
-		       "STA %pM erroneously requests BA session on tid %d w/o HT\n",
-		       sta->sta.addr, tid);
+		if (sta->sdata) {
+			ht_dbg(sta->sdata,
+			       "STA %pM erroneously requests BA session on tid %d w/o HT\n",
+			       sta->sta.addr, tid);
+		}
 		/* send a response anyway, it's an error case if we get here */
 		goto end;
 	}
 
 	if (test_sta_flag(sta, WLAN_STA_BLOCK_BA)) {
-		ht_dbg(sta->sdata,
-		       "Suspend in progress - Denying ADDBA request (%pM tid %d)\n",
-		       sta->sta.addr, tid);
+		if (sta->sdata) {
+			ht_dbg(sta->sdata,
+			       "Suspend in progress - Denying ADDBA request (%pM tid %d)\n",
+			       sta->sta.addr, tid);
+		}
 		goto end;
 	}
 
@@ -308,9 +319,11 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 	     (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) ||
 	    (buf_size > max_buf_size)) {
 		status = WLAN_STATUS_INVALID_QOS_PARAM;
-		ht_dbg_ratelimited(sta->sdata,
-				   "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
-				   sta->sta.addr, tid, ba_policy, buf_size);
+		if (sta->sdata) {
+			ht_dbg_ratelimited(sta->sdata,
+					   "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n",
+					   sta->sta.addr, tid, ba_policy, buf_size);
+		}
 		goto end;
 	}
 	/* determine default buffer size */
@@ -322,8 +335,10 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 		buf_size = sta->sta.max_rx_aggregation_subframes;
 	params.buf_size = buf_size;
 
-	ht_dbg(sta->sdata, "AddBA Req buf_size=%d for %pM\n",
-	       buf_size, sta->sta.addr);
+	if (sta->sdata) {
+		ht_dbg(sta->sdata, "AddBA Req buf_size=%d for %pM\n",
+		       buf_size, sta->sta.addr);
+	}
 
 	/* examine state machine */
 	lockdep_assert_held(&sta->ampdu_mlme.mtx);
@@ -332,9 +347,11 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 		if (sta->ampdu_mlme.tid_rx_token[tid] == dialog_token) {
 			struct tid_ampdu_rx *tid_rx;
 
-			ht_dbg_ratelimited(sta->sdata,
-					   "updated AddBA Req from %pM on tid %u\n",
-					   sta->sta.addr, tid);
+			if (sta->sdata) {
+				ht_dbg_ratelimited(sta->sdata,
+						   "updated AddBA Req from %pM on tid %u\n",
+						   sta->sta.addr, tid);
+			}
 			/* We have no API to update the timeout value in the
 			 * driver so reject the timeout update if the timeout
 			 * changed. If it did not change, i.e., no real update,
@@ -350,9 +367,11 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 			goto end;
 		}
 
-		ht_dbg_ratelimited(sta->sdata,
-				   "unexpected AddBA Req from %pM on tid %u\n",
-				   sta->sta.addr, tid);
+		if (sta->sdata) {
+			ht_dbg_ratelimited(sta->sdata,
+					   "unexpected AddBA Req from %pM on tid %u\n",
+					   sta->sta.addr, tid);
+		}
 
 		/* delete existing Rx BA session on the same tid */
 		___ieee80211_stop_rx_ba_session(sta, tid, WLAN_BACK_RECIPIENT,
@@ -360,7 +379,7 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 						false);
 	}
 
-	if (ieee80211_hw_check(&local->hw, SUPPORTS_REORDERING_BUFFER)) {
+	if (sta->sdata && ieee80211_hw_check(&local->hw, SUPPORTS_REORDERING_BUFFER)) {
 		ret = drv_ampdu_action(local, sta->sdata, &params);
 		ht_dbg(sta->sdata,
 		       "Rx A-MPDU request on %pM tid %d result %d\n",
@@ -400,14 +419,16 @@ void ___ieee80211_start_rx_ba_session(struct sta_info *sta,
 	for (i = 0; i < buf_size; i++)
 		__skb_queue_head_init(&tid_agg_rx->reorder_buf[i]);
 
-	ret = drv_ampdu_action(local, sta->sdata, &params);
-	ht_dbg(sta->sdata, "Rx A-MPDU request on %pM tid %d result %d\n",
-	       sta->sta.addr, tid, ret);
-	if (ret) {
-		kfree(tid_agg_rx->reorder_buf);
-		kfree(tid_agg_rx->reorder_time);
-		kfree(tid_agg_rx);
-		goto end;
+	if (sta->sdata) {
+		ret = drv_ampdu_action(local, sta->sdata, &params);
+		ht_dbg(sta->sdata, "Rx A-MPDU request on %pM tid %d result %d\n",
+		       sta->sta.addr, tid, ret);
+		if (ret) {
+			kfree(tid_agg_rx->reorder_buf);
+			kfree(tid_agg_rx->reorder_time);
+			kfree(tid_agg_rx);
+			goto end;
+		}
 	}
 
 	/* update data */
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index f9514bacbd4a..db53dcaccba9 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);
+	}
 
 	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 (WARN_ON(!sdata))
+		return;
+
+	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 (WARN_ON(!sdata))
+		return;
+
 	ht_dbg(sdata, "Stopping Tx BA session for %pM tid %d\n",
 	       sta->sta.addr, tid);
 
-- 
2.34.1
Re: [PATCH v3] net: mac80211: Add NULL checks for sta->sdata
Posted by Johannes Berg 2 years, 8 months ago 
On Tue, 2023-04-04 at 20:47 +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.

Right.

> However, in the same call stack, sta->sdata is also used in the
> following functions:
> 

Fun, guess we should fix that too then.

Honestly though, I don't think this patch is the right way of going
about it. It seems that instead we should expand the checks in the
previous patch - see how it's talking about a race, and we don't really
want or need to handle aggregation for a station that's being removed.

So I think the better way to fix it would be to prevent the race more
clearly, though off the top of my head I'm not sure how we'd do that.

johannes
Re: [PATCH v3] net: mac80211: Add NULL checks for sta->sdata
Posted by Simon Horman 2 years, 8 months ago 
On Tue, Apr 04, 2023 at 08:47:34PM +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>

Reviewed-by: Simon Horman <simon.horman@corigine.com>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.