Message-ID: <5e378fe7-90ec-4453-b549-1106f9d0cfef@quicinc.com>
Date: Tue, 24 Jun 2025 20:33:36 +0800
From: Zhongqiu Han <quic_zhonhan@...cinc.com>
To: Johannes Berg <johannes@...solutions.net>,
        <miriam.rachel.korenblit@...el.com>
CC: <linux-wireless@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <syzbot+5a7b40bcb34dea5ca959@...kaller.appspotmail.com>,
        Zhongqiu Han
	<quic_zhonhan@...cinc.com>
Subject: Re: [PATCH v3] wifi: mac80211: Prevent disconnect reports when no AP
 is associated

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@...solutions.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

Powered by blists - more mailing lists