| Message ID | 20250314120614.4032434-1-quic_zhonhan@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] wifi: mac80211: Prevent disconnect reports when no AP is associated | expand |
On Fri, 2025-03-14 at 20:06 +0800, Zhongqiu Han wrote: > > +++ 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; You're adding a WARN_ON() that's now guaranteed to trigger, no? Shouldn't the caller (also) be fixed? > @@ -8090,7 +8094,7 @@ static void ieee80211_sta_timer(struct timer_list *t) > void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, > u8 reason, bool tx) > { > - u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; > + u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN] = {0}; > And that's not needed then? And perhaps should be {} if it is. johannes
On 4/23/2025 11:26 PM, Johannes Berg wrote: > On Fri, 2025-03-14 at 20:06 +0800, Zhongqiu Han wrote: >> >> +++ 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; > > You're adding a WARN_ON() that's now guaranteed to trigger, no? > Shouldn't the caller (also) be fixed? Thanks Johannes for the review~ yes, the caller can first check if ap_sta is null, thereby preventing further calls to ieee80211_set_disassoc and ieee80211_report_disconnect, maybe just like the condition check for ifmgd->associated in the example below: 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))--------> introduced since 687a7c8a7227 return; if (WARN_ON_ONCE(tx && !frame_buf)) return; if (WARN_ON(!ifmgd->associated)) return; ... ... } ---example 1. ---------------------------------------------------------------- void ieee80211_sta_restart(struct ieee80211_sub_if_data *sdata) { struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; lockdep_assert_wiphy(sdata->local->hw.wiphy); if (!ifmgd->associated) return; if (sdata->flags & IEEE80211_SDATA_DISCONNECT_RESUME) { sdata->flags &= ~IEEE80211_SDATA_DISCONNECT_RESUME; mlme_dbg(sdata, "driver requested disconnect after resume\n"); ieee80211_sta_connection_lost(sdata, WLAN_REASON_UNSPECIFIED, true); return; } if (sdata->flags & IEEE80211_SDATA_DISCONNECT_HW_RESTART) { sdata->flags &= ~IEEE80211_SDATA_DISCONNECT_HW_RESTART; mlme_dbg(sdata, "driver requested disconnect after hardware restart\n"); ieee80211_sta_connection_lost(sdata, WLAN_REASON_UNSPECIFIED, true); return; } } ---------------------------------------------------------------------- ---example 2. ---------------------------------------------------------------------- static void ieee80211_rx_mgmt_deauth(struct ieee80211_sub_if_data *sdata, struct ieee80211_mgmt *mgmt, size_t len) { struct ieee80211_if_managed *ifmgd = &sdata->u.mgd; u16 reason_code = le16_to_cpu(mgmt->u.deauth.reason_code); lockdep_assert_wiphy(sdata->local->hw.wiphy); if (len < 24 + 2) return; if (!ether_addr_equal(mgmt->bssid, mgmt->sa)) { ieee80211_tdls_handle_disconnect(sdata, mgmt->sa, reason_code); return; } if (ifmgd->associated && ether_addr_equal(mgmt->bssid, sdata->vif.cfg.ap_addr)) { sdata_info(sdata, "deauthenticated from %pM (Reason: %u=%s)\n", sdata->vif.cfg.ap_addr, reason_code, ieee80211_get_reason_code_string(reason_code)); ieee80211_set_disassoc(sdata, 0, 0, false, NULL); ieee80211_report_disconnect(sdata, (u8 *)mgmt, len, false, reason_code, false); return; } ... ... ... } --------------------------------------------------------------------- However, sorry I'm not sure if !ifmgd->associated and !ap_sta have exactly the same context and treatment, especially in one situations like the below example3: ---example3. ----------------------------------------------------------------------- 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]; lockdep_assert_wiphy(local->hw.wiphy); if (!ifmgd->associated) return; if (!ifmgd->driver_disconnect) { unsigned int link_id; /* * AP is probably out of range (or not reachable for another * reason) so remove the bss structs for that AP. In the case * of multi-link, it's not clear that all of them really are * out of range, but if they weren't the driver likely would * have switched to just have a single link active? */ for (link_id = 0; link_id < ARRAY_SIZE(sdata->link); link_id++) { struct ieee80211_link_data *link; link = sdata_dereference(sdata->link[link_id], sdata); if (!link || !link->conf->bss) continue; cfg80211_unlink_bss(local->hw.wiphy, link->conf->bss); link->conf->bss = NULL; } } 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); ifmgd->reconnect = false; } ------------------------------------------------------------------------ My next step plan is change the code to as below: For example1 and example2 pattern, I want add more check about ap_sta such as: --------------------------------------------------------------------- if (ifmgd->associated && ether_addr_equal(mgmt->bssid, sdata->vif.cfg.ap_addr && ap_sta)) { --------------------------------------------------------------------- becasue if this condition meets, the if condition {} only execute ieee80211_set_disassoc and ieee80211_report_disconnect. For example3 pattern, i want to add check before the call of ieee80211_report_disconnect, but it is indeed not perfect. Thanks for your time and discussion~ > >> @@ -8090,7 +8094,7 @@ static void ieee80211_sta_timer(struct timer_list *t) >> void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, >> u8 reason, bool tx) >> { >> - u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; >> + u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN] = {0}; >> > > And that's not needed then? And perhaps should be {} if it is. Yes, it is not needed even {}. > > johannes
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c index c010bb3d24e3..08fb3fb740fd 100644 --- 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; if (tx) cfg80211_tx_mlme_mgmt(sdata->dev, buf, len, reconnect); @@ -8090,7 +8094,7 @@ static void ieee80211_sta_timer(struct timer_list *t) void ieee80211_sta_connection_lost(struct ieee80211_sub_if_data *sdata, u8 reason, bool tx) { - u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN]; + u8 frame_buf[IEEE80211_DEAUTH_FRAME_LEN] = {0}; ieee80211_set_disassoc(sdata, IEEE80211_STYPE_DEAUTH, reason, tx, frame_buf);
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 ===================================================== The reason is that the local variable frame_buf on the stack cannot be initialized by default. However one more question is that avoiding the uninit-value bug by explicitly initializing it is not enough. Since commit 687a7c8a7227 ("wifi: mac80211: change disassoc sequence a bit"), if there is no AP station, frame_buf has no chance to be assigned a valid value. The function ieee80211_report_disconnect should not continue executing with the frame_buf parameter that is merely initialized to zero. 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> --- 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 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)