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

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
2. Stick with the original approach of failing gracefully with -ENODEV
when DEVICE vif is unavailable?

My original fix (returning -ENODEV) does prevent the crash and allows
the system to continue operating normally, just without responding to
that particular iPhone query.

Best regards,
Richard