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.
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.
> 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.
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.
johannes
