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

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.

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

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