Re: [PATCH wireless-next] wifi: brcmfmac: fix crash while sending Action Frames in standalone AP Mode

On 10/12, Arend van Spriel wrote:
> On 10/9/2025 9:39 AM, Gokul Sivakumar wrote:
> > Currently, whenever there is a need to transmit an Action frame,
> > the brcmfmac driver always uses the P2P vif to send the "actframe" IOVAR to
> > firmware. The P2P interfaces were available when wpa_supplicant is managing
> > the wlan interface.
> > 
> > However, the P2P interfaces are not created/initialized when only hostapd
> > is managing the wlan interface. And if hostapd receives an ANQP Query REQ
> > Action frame even from an un-associated STA, the brcmfmac driver tries
> > to use an uninitialized P2P vif pointer for sending the IOVAR to firmware.
> > This NULL pointer dereferencing triggers a driver crash.
> > 
> >   [ 1417.074538] Unable to handle kernel NULL pointer dereference at virtual
> >   address 0000000000000000
> >   [...]
> >   [ 1417.075188] Hardware name: Raspberry Pi 4 Model B Rev 1.5 (DT)
> >   [...]
> >   [ 1417.075653] Call trace:
> >   [ 1417.075662]  brcmf_p2p_send_action_frame+0x23c/0xc58 [brcmfmac]
> >   [ 1417.075738]  brcmf_cfg80211_mgmt_tx+0x304/0x5c0 [brcmfmac]
> >   [ 1417.075810]  cfg80211_mlme_mgmt_tx+0x1b0/0x428 [cfg80211]
> >   [ 1417.076067]  nl80211_tx_mgmt+0x238/0x388 [cfg80211]
> >   [ 1417.076281]  genl_family_rcv_msg_doit+0xe0/0x158
> >   [ 1417.076302]  genl_rcv_msg+0x220/0x2a0
> >   [ 1417.076317]  netlink_rcv_skb+0x68/0x140
> >   [ 1417.076330]  genl_rcv+0x40/0x60
> >   [ 1417.076343]  netlink_unicast+0x330/0x3b8
> >   [ 1417.076357]  netlink_sendmsg+0x19c/0x3f8
> >   [ 1417.076370]  __sock_sendmsg+0x64/0xc0
> >   [ 1417.076391]  ____sys_sendmsg+0x268/0x2a0
> >   [ 1417.076408]  ___sys_sendmsg+0xb8/0x118
> >   [ 1417.076427]  __sys_sendmsg+0x90/0xf8
> >   [ 1417.076445]  __arm64_sys_sendmsg+0x2c/0x40
> >   [ 1417.076465]  invoke_syscall+0x50/0x120
> >   [ 1417.076486]  el0_svc_common.constprop.0+0x48/0xf0
> >   [ 1417.076506]  do_el0_svc+0x24/0x38
> >   [ 1417.076525]  el0_svc+0x30/0x100
> >   [ 1417.076548]  el0t_64_sync_handler+0x100/0x130
> >   [ 1417.076569]  el0t_64_sync+0x190/0x198
> >   [ 1417.076589] Code: f9401e80 aa1603e2 f9403be1 5280e483 (f9400000)
> > 
> > Fix this, by always using the vif corresponding to the wdev on which the
> > Action frame Transmission request was initiated by the userspace. This way,
> > even if P2P vif is not available, the IOVAR is sent to firmware on AP vif
> > and the ANQP Query RESP Action frame is transmitted without crashing the
> > driver.
> > 
> > Remove init_completion() for "send_af_done" from brcmf_p2p_create_p2pdev()
> > and do it in brcmf_p2p_tx_action_frame() instead of reinit_completion().
> > Because the formar function would not get executed when hostapd is managing
> > wlan interface, and so it is not safe to do reinit_completion() without any
> > prior init_completion().
> > 
> > And in the brcmf_p2p_tx_action_frame() function, the condition check for
> > P2P Presence response frame is not needed, since the wpa_supplicant is
> > properly sending the P2P Presense Response frame on the P2P-GO vif instead
> > of the P2P-Device vif.
> 
> Thanks, Gokul
> 
> Looks good. Respecting the vif provided by userspace is a better
> approach. So the actual issue is that hostapd does not create the
> P2P-Device interface, which is why I suggested to fallback on the
> primary vif. Overlooked the fact that we are doing a init_completion()
> in brcmf_p2p_create_p2pdev() which is needed for the action frame
> transmit. I have some comment on theat init_completion() change you made
> (see below).
> 
> Still would prefer to move the action frame transmit (afx) functionality
> out of the p2p source file, but that can be a separate rework.

I agree. The action frame transmission funcationality should properly work
for STA and AP mode, irrespective of P2P being enabled/disabled. So this
functionality needs to be cleaned up.

> > Fixes: 18e2f61db3b7 ("brcmfmac: P2P action frame tx.")
> > Signed-off-by: Gokul Sivakumar <[email protected]>
> > ---
> >   .../broadcom/brcm80211/brcmfmac/cfg80211.c    |  3 +-
> >   .../broadcom/brcm80211/brcmfmac/p2p.c         | 28 ++++++-------------
> >   .../broadcom/brcm80211/brcmfmac/p2p.h         |  3 +-
> >   3 files changed, 11 insertions(+), 23 deletions(-)
> [...]
> 
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> > index 0dc9d28cd77b..c7c40dc3be08 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> 
> [...]
> 
> > @@ -1538,28 +1539,20 @@ int brcmf_p2p_notify_action_tx_complete(struct brcmf_if *ifp,
> >    * The WLC_E_ACTION_FRAME_COMPLETE event will be received when the action
> >    * frame is transmitted.
> >    */
> > -static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
> > +static s32 brcmf_p2p_tx_action_frame(struct brcmf_if *ifp,
> > +                                  struct brcmf_p2p_info *p2p,
> >                                    struct brcmf_fil_af_params_le *af_params)
> >   {
> >       struct brcmf_pub *drvr = p2p->cfg->pub;
> > -     struct brcmf_cfg80211_vif *vif;
> > -     struct brcmf_p2p_action_frame *p2p_af;
> >       s32 err = 0;
> > 
> >       brcmf_dbg(TRACE, "Enter\n");
> > 
> > -     reinit_completion(&p2p->send_af_done);
> > +     init_completion(&p2p->send_af_done);
> 
> I think we should not do this here. It used to be init_completion() here
> but the kernel API changed introducing the reinit_completion() to make
> explicit distinction between init and reinit. So I would suggest to do
> the init_completion() in brcmf_p2p_attach() which is always invoked and
> leave the reinit_completion() here.

That works too. Will create an updated v2 patch after making this change
and submit it for the wireless tree instead of wireless-next.