Re: [PATCH wireless v1] wifi: mwifiex: discard erroneous disassoc frames on STA interface

[Vitor Soares <ivitro@gmail.com>]

On Tue, 2025-07-08 at 11:37 -0700, Brian Norris wrote:
> Hi Vitor,
> 
> On Tue, Jul 01, 2025 at 03:26:43PM +0100, Vitor Soares wrote:
> > From: Vitor Soares <vitor.soares@toradex.com>
> > 
> > When operating in concurrent STA/AP mode with host MLME enabled,
> > the firmware incorrectly sends disassociation frames to the STA
> > interface when clients disconnect from the AP interface.
> > This causes kernel warnings as the STA interface processes
> > disconnect events that don't apply to it:
> > 
> > [ 1303.240540] WARNING: CPU: 0 PID: 513 at net/wireless/mlme.c:141
> > cfg80211_process_disassoc+0x78/0xec [cfg80211]
> > [ 1303.250861] Modules linked in: 8021q garp stp mrp llc rfcomm bnep
> > btnxpuart nls_iso8859_1 nls_cp437 onboard_us
> > [ 1303.327651] CPU: 0 UID: 0 PID: 513 Comm: kworker/u9:2 Not tainted 6.16.0-
> > rc1+ #3 PREEMPT
> > [ 1303.335937] Hardware name: Toradex Verdin AM62 WB on Verdin Development
> > Board (DT)
> > [ 1303.343588] Workqueue: MWIFIEX_RX_WORK_QUEUE mwifiex_rx_work_queue
> > [mwifiex]
> > [ 1303.350856] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=-
> > -)
> > [ 1303.357904] pc : cfg80211_process_disassoc+0x78/0xec [cfg80211]
> > [ 1303.364065] lr : cfg80211_process_disassoc+0x70/0xec [cfg80211]
> > [ 1303.370221] sp : ffff800083053be0
> > [ 1303.373590] x29: ffff800083053be0 x28: 0000000000000000 x27:
> > 0000000000000000
> > [ 1303.380855] x26: 0000000000000000 x25: 00000000ffffffff x24:
> > ffff000002c5b8ae
> > [ 1303.388120] x23: ffff000002c5b884 x22: 0000000000000001 x21:
> > 0000000000000008
> > [ 1303.395382] x20: ffff000002c5b8ae x19: ffff0000064dd408 x18:
> > 0000000000000006
> > [ 1303.402646] x17: 3a36333a61623a30 x16: 32206d6f72662063 x15:
> > ffff800080bfe048
> > [ 1303.409910] x14: ffff000003625300 x13: 0000000000000001 x12:
> > 0000000000000000
> > [ 1303.417173] x11: 0000000000000002 x10: ffff000003958600 x9 :
> > ffff000003625300
> > [ 1303.424434] x8 : ffff00003fd9ef40 x7 : ffff0000039fc280 x6 :
> > 0000000000000002
> > [ 1303.431695] x5 : ffff0000038976d4 x4 : 0000000000000000 x3 :
> > 0000000000003186
> > [ 1303.438956] x2 : 000000004836ba20 x1 : 0000000000006986 x0 :
> > 00000000d00479de
> > [ 1303.446221] Call trace:
> > [ 1303.448722]  cfg80211_process_disassoc+0x78/0xec [cfg80211] (P)
> > [ 1303.454894]  cfg80211_rx_mlme_mgmt+0x64/0xf8 [cfg80211]
> > [ 1303.460362]  mwifiex_process_mgmt_packet+0x1ec/0x460 [mwifiex]
> > [ 1303.466380]  mwifiex_process_sta_rx_packet+0x1bc/0x2a0 [mwifiex]
> > [ 1303.472573]  mwifiex_handle_rx_packet+0xb4/0x13c [mwifiex]
> > [ 1303.478243]  mwifiex_rx_work_queue+0x158/0x198 [mwifiex]
> > [ 1303.483734]  process_one_work+0x14c/0x28c
> > [ 1303.487845]  worker_thread+0x2cc/0x3d4
> > [ 1303.491680]  kthread+0x12c/0x208
> > [ 1303.495014]  ret_from_fork+0x10/0x20
> > 
> > Add validation in the STA receive path to verify that disassoc/deauth
> > frames originate from the connected AP. Frames that fail this check
> > are discarded early, preventing them from reaching the MLME layer and
> > triggering WARN_ON().
> > 
> > This filtering logic is similar with that used in the
> > ieee80211_rx_mgmt_disassoc() function in mac80211, which drops
> > disassoc frames that don't match the current BSSID
> > (!ether_addr_equal(mgmt->bssid, sdata->vif.cfg.ap_addr)), ensuring
> > only relevant frames are processed.
> > 
> > Tested on:
> > - 8997 with FW 16.68.1.p197
> > 
> > Fixes: 36995892c271 ("wifi: mwifiex: add host mlme for client mode")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Vitor Soares <vitor.soares@toradex.com>
> > ---
> >  drivers/net/wireless/marvell/mwifiex/util.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/wireless/marvell/mwifiex/util.c
> > b/drivers/net/wireless/marvell/mwifiex/util.c
> > index 4c5b1de0e936..6882e90e90b2 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/util.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/util.c
> > @@ -459,7 +459,9 @@ mwifiex_process_mgmt_packet(struct mwifiex_private
> > *priv,
> >                                     "auth: receive authentication from
> > %pM\n",
> >                                     ieee_hdr->addr3);
> >                 } else {
> 
> How about the other leg of this 'else' (ieee80211_is_auth())? Is it
> possible for these spurious frames to accidentally look like our AUTH
> frames?

I haven't seen spurious frames matching the AUTH path so far. Also, if there's
no pending authentication on the STA, those frames should be already ignored.

> 
> > -                       if (!priv->wdev.connected)
> > +                       if (!priv->wdev.connected ||
> > +                           !ether_addr_equal(ieee_hdr->addr3,
> 
> "addr3" doesn't make it totally obvious to me what this actually means,
> nor that it's correct. Would ieee80211_get_SA(ieee_hdr) be equivalent?
> That seems a bit more descriptive. Or else maybe a short comment (e.g.,
> "ignore spurious management frames from other BSSIDs").

In this case, ieee80211_get_SA() wouldn't be equivalent and as far as I know,
there isn't a helper like ieee80211_get_SA() available.
This is an IBSS/Ad-hoc frame (To DS = 0, From DS = 0), so addr3 contains the
BSSID, which is what we need to match against (same as
ieee80211_rx_mgmt_disassoc()).

Since the patch was already merged, if you think a clarifying comment would
still be valuable, I can send a follow-up patch to add it.

> 
> It seems like it's correct, because this block already filters for
> specific frame types (auth, deauth, disassoc), but in isolation, it's
> not the most readable.
> 

Yes, I agree with you.

thanks,
Vitor

> Brian
> 
> > +                                             priv-
> > >curr_bss_params.bss_descriptor.mac_address))
> >                                 return 0;
> >  
> >                         if (ieee80211_is_deauth(ieee_hdr->frame_control)) {
> > -- 
> > 2.34.1
> >