Message-ID: <68ed28c9-66e8-4f7a-870c-97597f211548@quicinc.com>
Date: Wed, 25 Jun 2025 18:11:23 +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/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

Powered by blists - more mailing lists