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

On Wed, Oct 8, 2025 at 1:47 PM Arend van Spriel
<[email protected]> wrote:
>
> 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

Unfortunately, I don't have any logs for this test. When the bug occurs, the
Pi immediately becomes unstable and is unable to sync files to disk, so no
logs are preserved.

Richard