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@xxxxxxxxxxxxxxxx/
"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
