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

Hi Richard,

Disclaimer: my knowledge about the brcmfmac driver is very limited

Am 03.10.25 um 01:20 schrieb Richard Reigh:
> 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.
>
> Add NULL check for vif before dereferencing in brcmf_p2p_tx_action_frame()
> and return -ENODEV when vif is NULL. Also modify the retry loop in
> brcmf_p2p_send_action_frame() to stop immediately on permanent failures
> rather than retrying.
Since Arend's first suggestion didn't work, here is my feedback to your 
version in the case you still want to follow this approach here.
> Tested on Raspberry Pi Zero 2 W with kernel 6.1.21-v8+
This is the vendor kernel correct?
> Signed-off-by: Richard Reigh <[email protected]>
> ---
>   .../wireless/broadcom/brcm80211/brcmfmac/p2p.c   | 16 +++++++++++++++-
>   1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/p2p.c
> index 10d9d9c63..2c73156fa 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_err("vif is NULL, cannot send action frame\n");
Such errors are dedicated for real driver issues, but now we know that's 
possible to trigger this. It's okay for your tests, but at the end it 
should be a debug message.
> +		return -ENODEV;
> +	}
> +
I don't think this is the right place (tm), for the following reasons:
- the check is too late for an efficient bailout (e.g. there are msleeps 
before)
- we introduce a special return code for brcmf_p2p_tx_action_frame(), 
which makes it harder to maintain

So my suggestion would be to add the bailout in brcmf_cfg80211_mgmt_tx() 
if it's possible.

Best regards