[PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Zhongqiu Han]

syzbot reports that cfg80211_tx_mlme_mgmt is using uninit-value:

=====================================================
BUG: KMSAN: uninit-value in cfg80211_tx_mlme_mgmt+0x155/0x300 net/wireless/mlme.c:226
cfg80211_tx_mlme_mgmt+0x155/0x300 net/wireless/mlme.c:226
ieee80211_report_disconnect net/mac80211/mlme.c:4238 [inline]
ieee80211_sta_connection_lost+0xfa/0x150 net/mac80211/mlme.c:7811
ieee80211_sta_work+0x1dea/0x4ef0
ieee80211_iface_work+0x1900/0x1970 net/mac80211/iface.c:1684
cfg80211_wiphy_work+0x396/0x860 net/wireless/core.c:435
process_one_work kernel/workqueue.c:3236 [inline]
process_scheduled_works+0xc1a/0x1e80 kernel/workqueue.c:3317
worker_thread+0xea7/0x14f0 kernel/workqueue.c:3398
kthread+0x6b9/0xef0 kernel/kthread.c:464
ret_from_fork+0x6d/0x90 arch/x86/kernel/process.c:148
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

Local variable frame_buf created at:
ieee80211_sta_connection_lost+0x43/0x150 net/mac80211/mlme.c:7806
ieee80211_sta_work+0x1dea/0x4ef0
=====================================================

If ieee80211_report_disconnect() relies on the frame_buf initialized by
ieee80211_set_disassoc(), it must ensure that ieee80211_set_disassoc() has
called ieee80211_send_deauth_disassoc() to initialize it and has not
returned early. Since commit 687a7c8a7227 ("wifi: mac80211: change
disassoc sequence a bit"), ieee80211_set_disassoc() may return immediately
when no AP station is present, leaving frame_buf uninitialized.

To fix this issue, check the return value of ieee80211_set_disassoc()
before calling ieee80211_report_disconnect() if the latter relies on the
frame_buf initialized by the former.

Reported-by: syzbot+5a7b40bcb34dea5ca959@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/all/67bf36d3.050a0220.38b081.01ff.GAE@google.com/
Fixes: 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit")
Signed-off-by: Zhongqiu Han <quic_zhonhan@quicinc.com>
---
v2 -> v3:
- Rebased on top of current next.
- Update the v2 code implementation:
  - Remove zero-initialization of frame_buf
  - Remove WARN_ON and early return in ieee80211_report_disconnect()
  - Change the return type of ieee80211_set_disassoc(). If
    ieee80211_report_disconnect() uses the frame_buf initialized by
    ieee80211_set_disassoc(), its invocation is now conditional based
    on the return value of ieee80211_set_disassoc(). 
- Update commit message to reflect the modified code logic.
- Link to v2: https://lore.kernel.org/all/20250314120614.4032434-1-quic_zhonhan@quicinc.com/

v1 -> v2:
- Rebased on top of current next.
- Reorder the tags.
- Link to v1: https://lore.kernel.org/all/20250227090932.1871272-1-quic_zhonhan@quicinc.com/

 net/mac80211/mlme.c | 109 +++++++++++++++++++++++++++-----------------
 1 file changed, 67 insertions(+), 42 deletions(-)

diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 2d46d4af60d7..51750db61a54 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -3917,7 +3917,12 @@ static void ieee80211_ml_reconf_reset(struct ieee80211_sub_if_data *sdata)
 	}
 }
 
-static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
+/*
+ * Note that if ieee80211_report_disconnect() relies on the *frame_buf
+ * initialized by this function, then it must only be called if this function
+ * returns true; otherwise, it may use an uninitialized buffer.
+ */
+static bool ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 				   u16 stype, u16 reason, bool tx,
 				   u8 *frame_buf)
 {
@@ -3935,13 +3940,13 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 	lockdep_assert_wiphy(local->hw.wiphy);
 
 	if (WARN_ON(!ap_sta))
-		return;
+		return false;
 
 	if (WARN_ON_ONCE(tx && !frame_buf))
-		return;
+		return false;
 
 	if (WARN_ON(!ifmgd->associated))
-		return;
+		return false;
 
 	ieee80211_stop_poll(sdata);
 
@@ -4168,6 +4173,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
 
 	memset(ifmgd->userspace_selectors, 0,
 	       sizeof(ifmgd->userspace_selectors));
+
+	return true;
 }
 
 static void ieee80211_reset_ap_probe(struct ieee80211_sub_if_data *sdata)
@@ -4448,6 +4455,7 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 	struct ieee80211_local *local = sdata->local;
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
+	bool report_disconnect;
 
 	lockdep_assert_wiphy(local->hw.wiphy);
 
@@ -4477,20 +4485,22 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
 		}
 	}
 
-	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
-			       ifmgd->driver_disconnect ?
-					WLAN_REASON_DEAUTH_LEAVING :
-					WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
-			       true, frame_buf);
+	report_disconnect = ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
+						   ifmgd->driver_disconnect ?
+						   WLAN_REASON_DEAUTH_LEAVING :
+						   WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
+						   true, frame_buf);
 	/* the other links will be destroyed */
 	sdata->vif.bss_conf.csa_active = false;
 	sdata->deflink.u.mgd.csa.waiting_bcn = false;
 	sdata->deflink.u.mgd.csa.blocked_tx = false;
 	ieee80211_vif_unblock_queues_csa(sdata);
 
-	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
-				    WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
-				    ifmgd->reconnect);
+	if (report_disconnect)
+		ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf),
+					    true, WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
+					    ifmgd->reconnect);
+
 	ifmgd->reconnect = false;
 }
 
@@ -7477,9 +7487,12 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_link_data *link,
 	changed |= ieee80211_recalc_twt_req(sdata, sband, link, link_sta, elems);
 
 	if (ieee80211_config_bw(link, elems, true, &changed, "beacon")) {
-		ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
-				       WLAN_REASON_DEAUTH_LEAVING,
-				       true, deauth_buf);
+		if (!ieee80211_set_disassoc(sdata,
+					    IEEE80211_STYPE_DEAUTH,
+					    WLAN_REASON_DEAUTH_LEAVING,
+					    true, deauth_buf))
+			goto free;
+
 		ieee80211_report_disconnect(sdata, deauth_buf,
 					    sizeof(deauth_buf), true,
 					    WLAN_REASON_DEAUTH_LEAVING,
@@ -8090,8 +8103,9 @@ void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata,
 {
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 
-	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, reason,
-			       tx, frame_buf);
+	if (!ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, reason,
+				    tx, frame_buf))
+		return;
 
 	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
 				    reason, false);
@@ -8967,7 +8981,7 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_bss *bss;
 	u16 auth_alg;
 	int err;
-	bool cont_auth, wmm_used;
+	bool cont_auth, wmm_used, report_disconnect;
 
 	lockdep_assert_wiphy(sdata->local->hw.wiphy);
 
@@ -9089,14 +9103,16 @@ int ieee80211_mgd_auth(struct ieee80211_sub_if_data *sdata,
 		sdata_info(sdata,
 			   "disconnect from AP %pM for new auth to %pM\n",
 			   sdata->vif.cfg.ap_addr, auth_data->ap_addr);
-		ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
-				       WLAN_REASON_UNSPECIFIED,
-				       false, frame_buf);
+		report_disconnect = ieee80211_set_disassoc(sdata,
+							   IEEE80211_STYPE_DEAUTH,
+							   WLAN_REASON_UNSPECIFIED,
+							   false, frame_buf);
 
-		ieee80211_report_disconnect(sdata, frame_buf,
-					    sizeof(frame_buf), true,
-					    WLAN_REASON_UNSPECIFIED,
-					    false);
+		if (report_disconnect)
+			ieee80211_report_disconnect(sdata, frame_buf,
+						    sizeof(frame_buf), true,
+						    WLAN_REASON_UNSPECIFIED,
+						    false);
 	}
 
 	/* needed for transmitting the auth frame(s) properly */
@@ -9345,6 +9361,7 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 	struct cfg80211_bss *cbss;
 	bool override, uapsd_supported;
 	bool match_auth;
+	bool report_disconnect;
 	int i, err;
 	size_t size = sizeof(*assoc_data) + req->ie_len;
 
@@ -9392,14 +9409,16 @@ int ieee80211_mgd_assoc(struct ieee80211_sub_if_data *sdata,
 		sdata_info(sdata,
 			   "disconnect from AP %pM for new assoc to %pM\n",
 			   sdata->vif.cfg.ap_addr, assoc_data->ap_addr);
-		ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
-				       WLAN_REASON_UNSPECIFIED,
-				       false, frame_buf);
+		report_disconnect = ieee80211_set_disassoc(sdata,
+							   IEEE80211_STYPE_DEAUTH,
+							   WLAN_REASON_UNSPECIFIED,
+							   false, frame_buf);
 
-		ieee80211_report_disconnect(sdata, frame_buf,
-					    sizeof(frame_buf), true,
-					    WLAN_REASON_UNSPECIFIED,
-					    false);
+		if (report_disconnect)
+			ieee80211_report_disconnect(sdata, frame_buf,
+						    sizeof(frame_buf), true,
+						    WLAN_REASON_UNSPECIFIED,
+						    false);
 	}
 
 	memset(sdata->u.mgd.userspace_selectors, 0,
@@ -9720,6 +9739,7 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata,
 	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
 	bool tx = !req->local_state_change;
+	bool report_disconnect;
 	struct ieee80211_prep_tx_info info = {
 		.subtype = IEEE80211_STYPE_DEAUTH,
 	};
@@ -9773,11 +9793,14 @@ int ieee80211_mgd_deauth(struct ieee80211_sub_if_data *sdata,
 			   req->bssid, req->reason_code,
 			   ieee80211_get_reason_code_string(req->reason_code));
 
-		ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
-				       req->reason_code, tx, frame_buf);
-		ieee80211_report_disconnect(sdata, frame_buf,
-					    sizeof(frame_buf), true,
-					    req->reason_code, false);
+		report_disconnect = ieee80211_set_disassoc(sdata,
+							   IEEE80211_STYPE_DEAUTH,
+							   req->reason_code, tx, frame_buf);
+
+		if (report_disconnect)
+			ieee80211_report_disconnect(sdata, frame_buf,
+						    sizeof(frame_buf), true,
+						    req->reason_code, false);
 		return 0;
 	}
 
@@ -9788,6 +9811,7 @@ int ieee80211_mgd_disassoc(struct ieee80211_sub_if_data *sdata,
 			   struct cfg80211_disassoc_request *req)
 {
 	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
+	bool report_disconnect;
 
 	if (!sdata->u.mgd.associated ||
 	    memcmp(sdata->vif.cfg.ap_addr, req->ap_addr, ETH_ALEN))
@@ -9798,12 +9822,13 @@ int ieee80211_mgd_disassoc(struct ieee80211_sub_if_data *sdata,
 		   req->ap_addr, req->reason_code,
 		   ieee80211_get_reason_code_string(req->reason_code));
 
-	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DISASSOC,
-			       req->reason_code, !req->local_state_change,
-			       frame_buf);
+	report_disconnect = ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DISASSOC,
+						   req->reason_code, !req->local_state_change,
+						   frame_buf);
 
-	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
-				    req->reason_code, false);
+	if (report_disconnect)
+		ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
+					    req->reason_code, false);
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Johannes Berg]

On Fri, 2025-06-20 at 11:20 +0800, Zhongqiu Han wrote:
> 
> - Rebased on top of current next.
> - Update the v2 code implementation:
>   - Remove zero-initialization of frame_buf

You could keep that I guess, but we shouldn't claim it's any relation
with fixing the bug. Just as a cleanliness thing maybe?

>   - Remove WARN_ON and early return in ieee80211_report_disconnect()
>   - Change the return type of ieee80211_set_disassoc(). If
>     ieee80211_report_disconnect() uses the frame_buf initialized by
>     ieee80211_set_disassoc(), its invocation is now conditional based
>     on the return value of ieee80211_set_disassoc(). 

I don't understand this change ... surely syzbot couldn't have run into
an uninitialized buffer after the WARN_ON since it has panic_on_warn. So
why all these changes:

> -static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
> +/*
> + * Note that if ieee80211_report_disconnect() relies on the *frame_buf
> + * initialized by this function, then it must only be called if this function
> + * returns true; otherwise, it may use an uninitialized buffer.
> + */
> +static bool ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>  				   u16 stype, u16 reason, bool tx,
>  				   u8 *frame_buf)
>  {
> @@ -3935,13 +3940,13 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>  	lockdep_assert_wiphy(local->hw.wiphy);
>  
>  	if (WARN_ON(!ap_sta))
> -		return;
> +		return false;
>  
>  	if (WARN_ON_ONCE(tx && !frame_buf))
> -		return;
> +		return false;
>  
>  	if (WARN_ON(!ifmgd->associated))
> -		return;
> +		return false;
>  
>  	ieee80211_stop_poll(sdata);
>  
> @@ -4168,6 +4173,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>  
>  	memset(ifmgd->userspace_selectors, 0,
>  	       sizeof(ifmgd->userspace_selectors));
> +
> +	return true;
>  }

here to have a return value? It's only false when you had a WARN_ON()
which means there's a bug elsewhere?

>  static void ieee80211_reset_ap_probe(struct ieee80211_sub_if_data *sdata)
> @@ -4448,6 +4455,7 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
>  	struct ieee80211_local *local = sdata->local;
>  	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
>  	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
> +	bool report_disconnect;
>  
>  	lockdep_assert_wiphy(local->hw.wiphy);
>  
> @@ -4477,20 +4485,22 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
>  		}
>  	}
>  
> -	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
> -			       ifmgd->driver_disconnect ?
> -					WLAN_REASON_DEAUTH_LEAVING :
> -					WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
> -			       true, frame_buf);
> +	report_disconnect = ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
> +						   ifmgd->driver_disconnect ?
> +						   WLAN_REASON_DEAUTH_LEAVING :
> +						   WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
> +						   true, frame_buf);
>  	/* the other links will be destroyed */
>  	sdata->vif.bss_conf.csa_active = false;
>  	sdata->deflink.u.mgd.csa.waiting_bcn = false;
>  	sdata->deflink.u.mgd.csa.blocked_tx = false;
>  	ieee80211_vif_unblock_queues_csa(sdata);
>  
> -	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
> -				    WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
> -				    ifmgd->reconnect);
> +	if (report_disconnect)
> +		ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf),
> +					    true, WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
> +					    ifmgd->reconnect);
> +
>  	ifmgd->reconnect = false;

So all of that also doesn't really do anything.

But then the rest of the patch also doesn't seem to do anything, so what
am I missing?

Does the bug even still exist? Looking at the code now, I feel like
ccbaf782390d ("wifi: mac80211: rework the Tx of the deauth in
ieee80211_set_disassoc()") probably fixed this issue?

johannes

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Zhongqiu Han]

On 6/20/2025 7:13 PM, Johannes Berg wrote:
> On Fri, 2025-06-20 at 11:20 +0800, Zhongqiu Han wrote:
>>
>> - Rebased on top of current next.
>> - Update the v2 code implementation:
>>    - Remove zero-initialization of frame_buf
> 
> You could keep that I guess, but we shouldn't claim it's any relation
> with fixing the bug. Just as a cleanliness thing maybe?

Hi johannes
Thanks for the review~

yes, we can keep it.


> 
>>    - Remove WARN_ON and early return in ieee80211_report_disconnect()
>>    - Change the return type of ieee80211_set_disassoc(). If
>>      ieee80211_report_disconnect() uses the frame_buf initialized by
>>      ieee80211_set_disassoc(), its invocation is now conditional based
>>      on the return value of ieee80211_set_disassoc().
> 
> I don't understand this change ... surely syzbot couldn't have run into
> an uninitialized buffer after the WARN_ON since it has panic_on_warn. So
> why all these changes:

yes, syzbot couldn't have run into an uninitialized buffer after the
WARN_ON on **patch v2** such as:

--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -4433,6 +4433,10 @@ static void ieee80211_report_disconnect(struct 
ieee80211_sub_if_data *sdata,
  		.u.mlme.data = tx ? DEAUTH_TX_EVENT : DEAUTH_RX_EVENT,
  		.u.mlme.reason = reason,
  	};
+	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
+
+	if (WARN_ON(!ap_sta))
+		return;




Why did patch v3 override patch v2: Only check if the frame_buf has been
initialized by ieee80211_set_disassoc.

func ieee80211_report_disconnect() must use valid(initialized) frame_buf
to call cfg80211_tx_mlme_mgmt() or cfg80211_rx_mlme_mgmt(),

there are 2 condition:
condition 1: func ieee80211_report_disconnect() does not use a frame_buf
initialized by ieee80211_set_disassoc, so it is not affected by commit
687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit").

https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L4915
https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L4963
https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L9740
https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L9761


condition 2:.func ieee80211_report_disconnect() use a frame_buf
initialized by ieee80211_set_disassoc(), in such case, once
ieee80211_set_disassoc
early return and not call ieee80211_send_deauth_disassoc(), frame_buf
will be uninitialized. i want to fix this by current patch v3.

commit 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit").
add one more early return case:

https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L3936

static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
				   u16 stype, u16 reason, bool tx,
				   u8 *frame_buf)
{
	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
	struct ieee80211_local *local = sdata->local;
	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
	unsigned int link_id;
	u64 changed = 0;
	struct ieee80211_prep_tx_info info = {
		.subtype = stype,
		.was_assoc = true,
		.link_id = ffs(sdata->vif.active_links) - 1,
	};

	lockdep_assert_wiphy(local->hw.wiphy);

	if (WARN_ON(!ap_sta))--------------->commit 687a7c8a7227
		return;

	if (WARN_ON_ONCE(tx && !frame_buf))
		return;

	if (WARN_ON(!ifmgd->associated))-------------> the caller of
ieee80211_set_disassoc and ieee80211_report_disconnect has check the
this case.
		return;

so as long as ieee80211_set_disassoc() early return, it maybe better not
to call ieee80211_report_disconnect().

from your comments on patch v2:
https://lore.kernel.org
all/4ed3cfbc1a5b80bb3577f73b8c2b19ce830eeff5.camel@sipsolutions.net/

"You're adding a WARN_ON() that's now guaranteed to trigger, no
Shouldn't the caller (also) be fixed?
"
I would also like to add a check (!ap_sta) in the caller of
ieee80211_report_disconnect(), but the logic appears to be somewhat
complex, because I'm not certain if it is evaluated together with the (
ifmgd->associated) condition. I'm worried it might introduce new bugs.

https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L4946
https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L9085
https://elixir.bootlin.com/linux/v6.15.3/source/net/mac80211/mlme.c#L4907

in summary, why did patch v3 override patch v2:
(1) When ieee80211_report_disconnect uses a frame_buf initialized by
ieee80211_set_disassoc, we determine whether to call
ieee80211_report_disconnect based on whether ieee80211_set_disassoc has
already initialized it. This approach is more comprehensive than the one
in patch v2, which only checks WARN_ON(!ap_sta) inside
ieee80211_report_disconnect. It also allows for an early check before
calling ieee80211_report_disconnect.

(2) According to your comments on patch v2, it might also be necessary
to perform the check in the caller of ieee80211_report_disconnect.
However, if we simply check (!ap_sta), I'm concerned it could introduce
new issues if it's not clear where exactly that check should be placed.


> 
>> -static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>> +/*
>> + * Note that if ieee80211_report_disconnect() relies on the *frame_buf
>> + * initialized by this function, then it must only be called if this function
>> + * returns true; otherwise, it may use an uninitialized buffer.
>> + */
>> +static bool ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>>   				   u16 stype, u16 reason, bool tx,
>>   				   u8 *frame_buf)
>>   {
>> @@ -3935,13 +3940,13 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>>   	lockdep_assert_wiphy(local->hw.wiphy);
>>   
>>   	if (WARN_ON(!ap_sta))
>> -		return;
>> +		return false;
>>   
>>   	if (WARN_ON_ONCE(tx && !frame_buf))
>> -		return;
>> +		return false;
>>   
>>   	if (WARN_ON(!ifmgd->associated))
>> -		return;
>> +		return false;
>>   
>>   	ieee80211_stop_poll(sdata);
>>   
>> @@ -4168,6 +4173,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
>>   
>>   	memset(ifmgd->userspace_selectors, 0,
>>   	       sizeof(ifmgd->userspace_selectors));
>> +
>> +	return true;
>>   }
> 
> here to have a return value? It's only false when you had a WARN_ON()
> which means there's a bug elsewhere?

Maybe there is some misunderstanding.
in patch v2, WARN_ON() is added in ieee80211_report_disconnect() to
direct avoid use uninitialized frame_buf.

in patch v3, add return value on ieee80211_set_disassoc() to determine
if frame_buf has been initialized. if return false, will not call
ieee80211_report_disconnect().


> 
>>   static void ieee80211_reset_ap_probe(struct ieee80211_sub_if_data *sdata)
>> @@ -4448,6 +4455,7 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
>>   	struct ieee80211_local *local = sdata->local;
>>   	struct ieee80211_if_managed *ifmgd = &sdata->u.mgd;
>>   	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];
>> +	bool report_disconnect;
>>   
>>   	lockdep_assert_wiphy(local->hw.wiphy);
>>   
>> @@ -4477,20 +4485,22 @@ static void __ieee80211_disconnect(struct ieee80211_sub_if_data *sdata)
>>   		}
>>   	}
>>   
>> -	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
>> -			       ifmgd->driver_disconnect ?
>> -					WLAN_REASON_DEAUTH_LEAVING :
>> -					WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
>> -			       true, frame_buf);
>> +	report_disconnect = ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH,
>> +						   ifmgd->driver_disconnect ?
>> +						   WLAN_REASON_DEAUTH_LEAVING :
>> +						   WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
>> +						   true, frame_buf);
>>   	/* the other links will be destroyed */
>>   	sdata->vif.bss_conf.csa_active = false;
>>   	sdata->deflink.u.mgd.csa.waiting_bcn = false;
>>   	sdata->deflink.u.mgd.csa.blocked_tx = false;
>>   	ieee80211_vif_unblock_queues_csa(sdata);
>>   
>> -	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
>> -				    WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
>> -				    ifmgd->reconnect);
>> +	if (report_disconnect)
>> +		ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf),
>> +					    true, WLAN_REASON_DISASSOC_DUE_TO_INACTIVITY,
>> +					    ifmgd->reconnect);
>> +
>>   	ifmgd->reconnect = false;
> 
> So all of that also doesn't really do anything.

Maybe there is misunderstanding, if ieee80211_set_disassoc return false,
it avoids using an uninitialized frame_buf by not calling
ieee80211_report_disconnect. In patch v2, this was handled by adding a
separate (!ap_sta) check inside ieee80211_report_disconnect to prevent
further execution.

Both approaches effectively prevent the current syzbot-reported bug.

> 
> But then the rest of the patch also doesn't seem to do anything, so what
> am I missing?
> 
> Does the bug even still exist? Looking at the code now, I feel like
> ccbaf782390d ("wifi: mac80211: rework the Tx of the deauth in
> ieee80211_set_disassoc()") probably fixed this issue?

this commit should not. What we're currently concerned about is how to
prevent ieee80211_set_disassoc from returning early before
ieee80211_send_deauth_disassoc is called to initialize frame_buf.



ccbaf782390d ("wifi: mac80211: rework the Tx of the deauth in
 > ieee80211_set_disassoc()") partial change:
-       if (tx)
-               ieee80211_flush_queues(local, sdata, true);
+       ieee80211_flush_queues(local, sdata, true);

-       /* deauthenticate/disassociate now */
-       if (tx || frame_buf) {
+       if (tx) {
                 drv_mgd_prepare_tx(sdata->local, sdata, &info);

                 ieee80211_send_deauth_disassoc(sdata, 
sdata->vif.cfg.ap_addr,
                                                sdata->vif.cfg.ap_addr, 
stype,
-                                              reason, tx, frame_buf);
-       }
+                                              reason, true, frame_buf);

-       /* flush out frame - make sure the deauth was actually sent */
-       if (tx)
+               /* flush out frame - make sure the deauth was actually 
sent */
                 ieee80211_flush_queues(local, sdata, false);

-       if (tx || frame_buf)
                 drv_mgd_complete_tx(sdata->local, sdata, &info);
+       } else if (frame_buf) {
+               ieee80211_send_deauth_disassoc(sdata, 
sdata->vif.cfg.ap_addr,
+                                              sdata->vif.cfg.ap_addr, 
stype,
+                                              reason, false, frame_buf);
+       }


If ieee80211_set_disassoc is responsible for initializing frame_buf,
then the address of frame_buf will definitely not be null. Regardless of
whether tx is true or not, ieee80211_send_deauth_disassoc will be
called, and frame_buf will be initialized.
Our goal is to ensure that ieee80211_set_disassoc does not return
prematurely before ieee80211_send_deauth_disassoc is invoked.

Besides, maybe patch v2 should also fix the issue. Please kindly let me
the update~

Thank you for your time and the discussion~



> 
> johannes


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Johannes Berg]

> > 
> > >    - Remove WARN_ON and early return in ieee80211_report_disconnect()
> > >    - Change the return type of ieee80211_set_disassoc(). If
> > >      ieee80211_report_disconnect() uses the frame_buf initialized by
> > >      ieee80211_set_disassoc(), its invocation is now conditional based
> > >      on the return value of ieee80211_set_disassoc().
> > 
> > I don't understand this change ... surely syzbot couldn't have run into
> > an uninitialized buffer after the WARN_ON since it has panic_on_warn. So
> > why all these changes:
> 
> yes, syzbot couldn't have run into an uninitialized buffer after the
> WARN_ON on **patch v2** such as:
> 
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -4433,6 +4433,10 @@ static void ieee80211_report_disconnect(struct 
> ieee80211_sub_if_data *sdata,
>   		.u.mlme.data = tx ? DEAUTH_TX_EVENT : DEAUTH_RX_EVENT,
>   		.u.mlme.reason = reason,
>   	};
> +	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
> +
> +	if (WARN_ON(!ap_sta))
> +		return;

I think you misunderstood ... We have this WARN_ON since 687a7c8a7227
("wifi: mac80211: change disassoc sequence a bit"). Therefore, !ap_sta
cannot be the cause of syzbot complaints, since WARN_ON would panic it
before it ever gets to the uninitialized memory use.


> "You're adding a WARN_ON() that's now guaranteed to trigger, no

so now it's no longer your WARN_ON, I guess, but how did it trigger? I
really think we need to figure out how it triggered and fix _that_.

johannes

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Zhongqiu Han]

On 6/24/2025 9:35 PM, Johannes Berg wrote:
>>>
>>>>     - Remove WARN_ON and early return in ieee80211_report_disconnect()
>>>>     - Change the return type of ieee80211_set_disassoc(). If
>>>>       ieee80211_report_disconnect() uses the frame_buf initialized by
>>>>       ieee80211_set_disassoc(), its invocation is now conditional based
>>>>       on the return value of ieee80211_set_disassoc().
>>>
>>> I don't understand this change ... surely syzbot couldn't have run into
>>> an uninitialized buffer after the WARN_ON since it has panic_on_warn. So
>>> why all these changes:
>>
>> yes, syzbot couldn't have run into an uninitialized buffer after the
>> WARN_ON on **patch v2** such as:
>>
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -4433,6 +4433,10 @@ static void ieee80211_report_disconnect(struct
>> ieee80211_sub_if_data *sdata,
>>    		.u.mlme.data = tx ? DEAUTH_TX_EVENT : DEAUTH_RX_EVENT,
>>    		.u.mlme.reason = reason,
>>    	};
>> +	struct sta_info *ap_sta = sta_info_get(sdata, sdata->vif.cfg.ap_addr);
>> +
>> +	if (WARN_ON(!ap_sta))
>> +		return;
> 
> I think you misunderstood ... We have this WARN_ON since 687a7c8a7227
> ("wifi: mac80211: change disassoc sequence a bit"). Therefore, !ap_sta
> cannot be the cause of syzbot complaints, since WARN_ON would panic it
In my experience, WARN_ON is rarely configured to trigger a panic.
> before it ever gets to the uninitialized memory use.
> 
Hi johannes
Thanks a lot for your discussion and review~

 >>>
We have this WARN_ON since 687a7c8a7227
("wifi: mac80211: change disassoc sequence a bit")
 >>>

this WARN_ON was added in func ieee80211_set_disassoc() but not 
ieee80211_report_disconnect()
I add WARN_ON on ieee80211_report_disconnect() on my patch v2,

It was precisely because of the WARN_ON at 687a7c8a7227 that caused
ieee80211_set_disassoc to return early(when panic_on_warn is not
enabled). As a result, ieee80211_set_disassoc failed to properly
initialize frame_buf. Then, when ieee80211_report_disconnect was called,
it ended up using an uninitialized value.

> 
>> "You're adding a WARN_ON() that's now guaranteed to trigger, no
> 
> so now it's no longer your WARN_ON, I guess, but how did it trigger? I
> really think we need to figure out how it triggered and fix _that_.
> 

The bug was triggered as follow:

Commit 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit") 
introduced a code path where ieee80211_set_disassoc may return early if 
WARN_ON(!ap_sta) is triggered(panic_on_warn is not enabled). In this 
case, frame_buf is not initialized.

Later, when ieee80211_report_disconnect is called, it attempts to use 
the uninitialized frame_buf, resulting in a bug.

This is the reason I tagged:
Fixes: 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit")


In my patch v2, I want to fix the bug by adding "WARN_ON(!ap_sta) and 
return" on  ieee80211_report_disconnect() to avoid continue use frame_buf.

In my patch v3, I plan to fix the bug by avoid calling 
ieee80211_report_disconnect() when frame_buf is not initialized by 
ieee80211_set_disassoc()


> johannes


-- 
Thx and BRs,
Zhongqiu Han

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Johannes Berg]

On Wed, 2025-06-25 at 11:58 +0800, Zhongqiu Han wrote:
> 
>  >>>
> We have this WARN_ON since 687a7c8a7227
> ("wifi: mac80211: change disassoc sequence a bit")
>  >>>
> 
> this WARN_ON was added in func ieee80211_set_disassoc() but not 
> ieee80211_report_disconnect()
> I add WARN_ON on ieee80211_report_disconnect() on my patch v2,
> 
> It was precisely because of the WARN_ON at 687a7c8a7227 that caused
> ieee80211_set_disassoc to return early(when panic_on_warn is not
> enabled). As a result, ieee80211_set_disassoc failed to properly
> initialize frame_buf. Then, when ieee80211_report_disconnect was called,
> it ended up using an uninitialized value.

Sure, but now you're talking about something that's not supposed to
happen. The WARN there means "someone made a mistake in this code". I'm
not even super concerned about using uninitialised memory in that case
although I agree we should avoid it, so the trivial fix for the data
leak, without making the logic more complex, would be to just initialise
the data to zero. Not to fix the syzbot issue (which btw is already no
longer happening according to the dashboard), but to fix the potential
data leak.

Since ieee80211_set_disassoc() returns early only with a WARN, with
syzbot you won't ever get to the uninitialized memory use though, which
is what I was asking.

> The bug was triggered as follow:
> 
> Commit 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit") 
> introduced a code path where ieee80211_set_disassoc may return early if 
> WARN_ON(!ap_sta) is triggered(panic_on_warn is not enabled). In this 
> case, frame_buf is not initialized.

No ... that can't be right, we _know_ that syzbot enables panic_on_warn.
So this particular bug report from syzbot is definitely _not_ triggered
this way.

> Later, when ieee80211_report_disconnect is called, it attempts to use 
> the uninitialized frame_buf, resulting in a bug.

I agree this is a bug but it's not one that requires major surgery to
fix - the only bug here is that if we already have another bug that
leads to the WARN, we can leak stack data to userspace. We can simply
initialise the data to avoid that. I'm not worried about reporting a bad
thing to userspace if we already had a WARN indicating some _other_ bug
that we should fix.


I'll happily take a patch that says "initialise the frame buffer so that
on the off chance of other bugs, we don't leak stack data to userspace",
but I do think that's about all we need at this point. The syzbot report
is already stale and no longer happening anyway. If there is another
report about the WARN_ON after commit ccbaf782390d ("wifi: mac80211:
rework the Tx of the deauth in ieee80211_set_disassoc()"), we need to
see how it's possible that we get into the WARN case, but I haven't seen
such a report.

johannes

Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP is associated

[Zhongqiu Han]

On 6/25/2025 2:45 PM, Johannes Berg wrote:
> On Wed, 2025-06-25 at 11:58 +0800, Zhongqiu Han wrote:
>>
>>   >>>
>> We have this WARN_ON since 687a7c8a7227
>> ("wifi: mac80211: change disassoc sequence a bit")
>>   >>>
>>
>> this WARN_ON was added in func ieee80211_set_disassoc() but not
>> ieee80211_report_disconnect()
>> I add WARN_ON on ieee80211_report_disconnect() on my patch v2,
>>
>> It was precisely because of the WARN_ON at 687a7c8a7227 that caused
>> ieee80211_set_disassoc to return early(when panic_on_warn is not
>> enabled). As a result, ieee80211_set_disassoc failed to properly
>> initialize frame_buf. Then, when ieee80211_report_disconnect was called,
>> it ended up using an uninitialized value.
> 
> Sure, but now you're talking about something that's not supposed to
> happen. The WARN there means "someone made a mistake in this code". I'm
> not even super concerned about using uninitialised memory in that case
> although I agree we should avoid it, so the trivial fix for the data
> leak, without making the logic more complex, would be to just initialise
> the data to zero. Not to fix the syzbot issue (which btw is already no
> longer happening according to the dashboard), but to fix the potential
> data leak.

Hi johannes~

Thanks for your discussion~

I really appreciate your review and don’t want to take up too much of
your time.

yes, potential data leak issue should be fixed.

> 
> Since ieee80211_set_disassoc() returns early only with a WARN, with
> syzbot you won't ever get to the uninitialized memory use though, which
> is what I was asking.
> 
>> The bug was triggered as follow:
>>
>> Commit 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit")
>> introduced a code path where ieee80211_set_disassoc may return early if
>> WARN_ON(!ap_sta) is triggered(panic_on_warn is not enabled). In this
>> case, frame_buf is not initialized.
> 
> No ... that can't be right, we _know_ that syzbot enables panic_on_warn.
> So this particular bug report from syzbot is definitely _not_ triggered
> this way.

Let's revisit the initial stack trace where the problem originated.
the issue occurred on commit:
HEAD commit:    ff202c5028a1 Merge tag 'soc-fixes-6.14' of git://git.kerne..

=====================================================
BUG: KMSAN: uninit-value in cfg80211_tx_mlme_mgmt+0x155/0x300 
net/wireless/mlme.c:226
cfg80211_tx_mlme_mgmt+0x155/0x300 net/wireless/mlme.c:226
ieee80211_report_disconnect net/mac80211/mlme.c:4238 [inline]
ieee80211_sta_connection_lost+0xfa/0x150 net/mac80211/mlme.c:7811
ieee80211_sta_work+0x1dea/0x4ef0
ieee80211_iface_work+0x1900/0x1970 net/mac80211/iface.c:1684
cfg80211_wiphy_work+0x396/0x860 net/wireless/core.c:435
process_one_work kernel/workqueue.c:3236 [inline]
process_scheduled_works+0xc1a/0x1e80 kernel/workqueue.c:3317
worker_thread+0xea7/0x14f0 kernel/workqueue.c:3398
kthread+0x6b9/0xef0 kernel/kthread.c:464
ret_from_fork+0x6d/0x90 arch/x86/kernel/process.c:148
ret_from_fork_asm+0x1a/0x30 arch/x86/entry/entry_64.S:244

Local variable frame_buf created at:
ieee80211_sta_connection_lost+0x43/0x150 net/mac80211/mlme.c:7806
ieee80211_sta_work+0x1dea/0x4ef0
=====================================================

frame_buf is created at ieee80211_sta_connection_lost()
and the func is short:


void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata,
				   u8 reason, bool tx)
{
	u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN];

	ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, reason,
			       tx, frame_buf);

	ieee80211_report_disconnect(sdata, frame_buf, sizeof(frame_buf), true,
				    reason, false);
}


In func ieee80211_set_disassoc(), frame_buf will never be NULL, so
ieee80211_send_deauth_disassoc must be called if
ieee80211_set_disassoc() not return early.


	/* deauthenticate/disassociate now */
	if (tx || frame_buf) {
		drv_mgd_prepare_tx(sdata->local, sdata, &info);

		ieee80211_send_deauth_disassoc(sdata, sdata->vif.cfg.ap_addr,
					       sdata->vif.cfg.ap_addr, stype,
					       reason, tx, frame_buf);
	}


I also checked the cmdline from kconfig, there is no panic_on_warn on
kernel config:  https://syzkaller.appspot.com/x/.config
x=aca5947365998f67

However there is possible it is partial or not used cmdline as
CONFIG_CMDLINE_FORCE is not set.

CONFIG_CMDLINE="earlyprintk=serial net.ifnames=0
sysctl.kernel.hung_task_all_cpu_backtrace=1 ima_policy=tcb nf-conntrack
ftp.ports=20000 nf-conntrack-tftp.ports=20000 nf-conntrack
sip.ports=20000 nf-conntrack-irc.ports=20000 nf-conntrack
sane.ports=20000 binder.debug_mask=0 rcupdate.rcu_expedited=1
rcupdate.rcu_cpu_stall_cputime=1 no_hash_pointers page_owner=on
sysctl.vm.nr_hugepages=4 sysctl.vm.nr_overcommit_hugepages=4
secretmem.enable=1 sysctl.max_rcu_stall_to_panic=1 msr.allow_writes=off
coredump_filter=0xffff root=/dev/sda console=ttyS0 vsyscall=native
numa=fake=2 kvm-intel.nested=1 spec_store_bypass_disable=prctl nopcid
vivid.n_devs=64
vivid.multiplanar=1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2,1,2
netrom.nr_ndevs=32 rose.rose_ndevs=32 smp.csd_lock_timeout=100000
watchdog_thresh=55 workqueue.watchdog_thresh=140
sysctl.net.core.netdev_unregister_timeout_secs=140 dummy_hcd.num=32
max_loop=32 nbds_max=32 kmsan.panic=1"


Although I suspect the if the syzbot has indeed enabled panic_on_warn or
not,  the most important thing is to avoid data leak and also what I’m
concerned about might also trigger a WARN_ON on ieee80211_set_disassoc.


> 
>> Later, when ieee80211_report_disconnect is called, it attempts to use
>> the uninitialized frame_buf, resulting in a bug.
> 
> I agree this is a bug but it's not one that requires major surgery to
> fix - the only bug here is that if we already have another bug that
> leads to the WARN, we can leak stack data to userspace. We can simply
> initialise the data to avoid that. I'm not worried about reporting a bad
> thing to userspace if we already had a WARN indicating some _other_ bug
> that we should fix.

Sure, I got you point~ maybe the only thing we need to do is avoiding
data leak to userspace side.


> 
> 
> I'll happily take a patch that says "initialise the frame buffer so that
> on the off chance of other bugs, we don't leak stack data to userspace",
> but I do think that's about all we need at this point. The syzbot report
> is already stale and no longer happening anyway. If there is another
> report about the WARN_ON after commit ccbaf782390d ("wifi: mac80211:
> rework the Tx of the deauth in ieee80211_set_disassoc()"), we need to
> see how it's possible that we get into the WARN case, but I haven't seen
> such a report.

Sure, I plan to arise one such patch to initialize all the frame_buf on
net/mac80211/mlme.c file.

Thanks johannes for the nice discussion~

> 
> johannes


-- 
Thx and BRs,
Zhongqiu Han