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

On 10/6/2025 8:37 PM, Richard Reigh wrote:
> On Mon, Oct 6, 2025 at 12:28 PM Stefan Wahren <[email protected]> wrote:
>>
>> Am 06.10.25 um 19:08 schrieb Richard Reigh:
>>> On Mon, Oct 6, 2025 at 10:47 AM Stefan Wahren <[email protected]> wrote:
>>>> Hi Richard,
>>>>
>>>> Am 04.10.25 um 11:22 schrieb Arend van Spriel:
>>>>> On 10/3/2025 7:02 PM, Stefan Wahren wrote:
>>>>>> Hi,
>>>>>>
>>>>>> Am 03.10.25 um 18:00 schrieb Arend van Spriel:
>>>>>>> Op 3 oktober 2025 01:20:21 schreef Richard Reigh <[email protected]>:
>>>>>>>
>>>>>>>> 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.
>>>>>>> That does make a lot more sense than the line that gdb gave me with
>>>>>>> my driver rebuild attempt.
>>>>>>>
>>>>>>>> Add NULL check for vif before dereferencing in
>>>>>>>> brcmf_p2p_tx_action_frame()
>>>>>>>> and return -ENODEV when vif is NULL. Also modify
>>>>>>> Now I don't think it is necessary to bail out here. See my
>>>>>>> suggestion below...
>>>>>>>
>>>>>>>> the retry loop in
>>>>>>>> brcmf_p2p_send_action_frame() to stop immediately on permanent
>>>>>>>> failures
>>>>>>>> rather than retrying.
>>>>>>>>
>>>>>>>> Tested on Raspberry Pi Zero 2 W with kernel 6.1.21-v8+
>>>>>>>>
>>>>>>>> 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");
>>>>>>>> + return -ENODEV;
>>>>>>>> + }
>>>>>>> Instead of giving up and surrender we can use primary vif, ie. p2p-
>>>>>>>> bss_idx[P2PAPI_BSSCFG_PRIMARY].vif:
>>>>>>> if (!vif) {
>>>>>>>          brcmf_dbg(TRACE, "no P2P device setup\n");
>>>>>>>          vif = p2p->bss_idx[P2PAPI_BSSCFG_PRIMARY].vif;
>>>>>>> }
>>>>>> this looks much better. But the whole patch needs a fixes tag.
>>>>> Sure.
>>>>>
>>>>>> In case this issue is reproducible before Linux 5.8, the tag should
>>>>>> reference:
>>>>>> 18e2f61db3b7 ("brcmfmac: P2P action frame tx.")
>>>>>>
>>>>>> otherwise it should reference:
>>>>>> d524d5ce3655 ("brcmfmac: p2p cert 6.1.9-support GOUT handling p2p
>>>>>> presence request")
>>>>> Both good suggestions. Looking at the code I would say that
>>>>> 18e2f61db3b7 ("brcmfmac: P2P action frame tx.") is the best choice.
>>>> thanks. @Richard Do you plan to submit a V2 patch?
>>>>
>>>> Best regards
>>>>> Regards,
>>>>> Arend
>>> Stefan,
>>>
>>> Yes, I plan on submitting a V2 patch, after I finish testing the change.
>>>
>>> I will also add the suggested fixes tag to the patch submission.
>> Sure, take your time.
>>
>> Thanks
>>>
>>> Thanks,
>>> Richard
> 
> Hi Arend and Stefan,
> 
> Thank you for the feedback on V1. I've tested the suggested fallback
> to PRIMARY vif, but unfortunately it still crashes with a NULL pointer
> dereference.
> 
> The PRIMARY vif itself is not NULL (confirmed via debug output), but
> using it for P2P action frames causes a crash later in
> brcmf_p2p_tx_action_frame(). It appears the PRIMARY vif may not have
> the necessary P2P-specific structures initialized.
> 
> Should I:
> 
> 1. Investigate which specific pointer/structure is NULL when using
> PRIMARY vif for P2P operations, or

I would be interested to see the log of the "PRIMARY vif" tesing if you 
still have it.

Regards,
Arend