net/mac80211/mlme.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
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(-)
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);
--
2.25.1On 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
--
Thx and BRs,
Zhongqiu Han
© 2016 - 2025 Red Hat, Inc.