Re: [PATCH v2] brcmfmac: Fix NULL pointer dereference in P2P action frame handling

On 10/7/2025 5:30 PM, Gokul Sivakumar wrote:
> On 10/06, Richard Reigh wrote:
>> When iPhones query network information via the WiFi settings "i" button,
>> the brcmfmac driver crashes with a NULL pointer dereference. This occurs
>> because the P2P device vif is not initialized when handling certain action
>> frames.
>>
>> Added an early check in brcmf_cfg80211_mgmt_tx() to verify P2P device vif
>> availability before attepting to send action frames. Also added a defensive
>> check in brcmf_p2p_tx_action_frame() as a safety net.
>>
>> Tested on Raspberry Pi Zero 2 W with kernel 6.1.21-v8+
>>
>> Fixes: 18e2f61db3b7 ("brcmfmac: P2P action frame tx")
>> Signed-off-by: Richard Reigh <[email protected]>
>>
>> ---
>> Changes in v2:
>> - Added early check in cfg80211.c before calling P2P functions (suggested by
>>    Stefan Wahren)
>> - Changed error message to debug level in p2p.c (suggested by Stefan Wahren)
>> - Added fixes tag
>> ---
>>   .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 13 +++++++++++++
>>   .../net/wireless/broadcom/brcm80211/brcmfmac/p2p.c  |  6 ++++++
>>   2 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> index ea8409e0e..9983136a7 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
>> @@ -5137,6 +5137,19 @@ brcmf_cfg80211_mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev,
>>
>>          vif = container_of(wdev, struct brcmf_cfg80211_vif, wdev);
>>
>> +       /* check if P2P device vif is available before attempting */
>> +       /* action frames */
>> +       if (ieee80211_is_action(mgmt->frame_control)) {
> 
> The action frame type specific operations are done already below in the
> else if (ieee80211_is_action()) block. So ideally, it is better to do
> all action frame type specific operations in the existing else if{} block,
> instead of introducing a new if{} block, until unless it is really needed.
> 
>> +               struct brcmf_p2p_info *p2p = &cfg->p2p;
>> +
>> +               if (!p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif) {
>> +                       brcmf_dbg(TRACE, "P2P device vif not available\n");
>> +                       cfg80211_mgmt_tx_status(wdev, *cookie, buf, len, false,
>> +                                               GFP_KERNEL);
>> +                       return 0;
>> +               }
>> +       }
>> +
> 
> IMHO, bailing out here without responding to the ANQP Request, may not be
> a viable solution for the crash. Need to have a way for the AP to respond
> with the ANQP Response Public Action frame without crashing the driver.

Carying the historical burden. The action frame transmission support in 
brcmfmac was originally introduced with only P2P in mind. So it ended up 
very entwined with the P2P code. In my opinion the action frame handling 
should be implemented in a separate source file in the driver.

> Have a solution in mind, will send the patch for review after testing it.

Okay. Go ahead.

Gr. AvS

>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>> index 10d9d9c63..e62135cd4 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
>> @@ -1559,6 +1559,12 @@ static s32 brcmf_p2p_tx_action_frame(struct brcmf_p2p_info *p2p,
>>          else
>>                  vif = p2p->bss_idx[P2PAPI_BSSCFG_DEVICE].vif;
>>
>> +       /* add NULL check */
>> +       if (!vif) {
>> +               brcmf_dbg(TRACE, "vif is NULL, cannot send action frame\n");
>> +               return -ENODEV;
>> +       }
>> +
>>          err = brcmf_fil_bsscfg_data_set(vif->ifp, "actframe", af_params,
>>                                          sizeof(*af_params));
>>          if (err) {
>> --
>> 2.34.1
>>
>>