Hello, On Thu, Jan 09, 2025 at 08:33:37PM +0100, Jonas Gorski wrote: > CAUTION: This email comes from a non Wind River email account! > Do not click links or open attachments unless you recognize the sender and know the content is safe. > > Hi, > > On Thu, Jan 9, 2025 at 4:53 PM Marcel Hamer <marcel.hamer@xxxxxxxxxxxxx> wrote: > > > > On removal of the device or unloading of the kernel module a potential > > NULL pointer dereference occurs. > > > > The following sequence deletes the interface: > > > > brcmf_detach() > > brcmf_remove_interface() > > brcmf_del_if() > > > > Inside the brcmf_del_if() function the drvr->if2bss[ifidx] is updated to > > BRCMF_BSSIDX_INVALID (-1) if the bsscfgidx matches. > > > > After brcmf_remove_interface() call the brcmf_proto_detach() function is > > called providing the following sequence: > > > > brcmf_detach() > > brcmf_proto_detach() > > brcmf_proto_msgbuf_detach() > > brcmf_flowring_detach() > > brcmf_msgbuf_delete_flowring() > > brcmf_msgbuf_remove_flowring() > > brcmf_flowring_delete() > > brcmf_get_ifp() > > brcmf_txfinalize() > > > > Since brcmf_get_ip() can and actually will return NULL in this case the > > call to brcmf_txfinalize() will result in a NULL pointer dereference > > inside brcmf_txfinalize() when trying to update > > ifp->ndev->stats.tx_errors. > > > > This will only happen if a flowring still has an skb. > > > > Cc: stable@xxxxxxxxxxxxxxx > > Signed-off-by: Marcel Hamer <marcel.hamer@xxxxxxxxxxxxx> > > Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@xxxxxxxxx/ > > --- > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > > index c3a57e30c855..cf731bc7ae24 100644 > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c > > @@ -549,7 +549,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success) > > wake_up(&ifp->pend_8021x_wait); > > Here is some additional potential ifp access, which happens if the > ethtype is ETH_P_PAE. Should this also be guarded? To me it looks it > might also break there, just with lower probability. Or is this > impossible to reach when detaching? > > > } > > > > - if (!success) > > + if (!success && ifp) > > ifp->ndev->stats.tx_errors++; > > > > brcmu_pkt_buf_free_skb(txp); > > Best Regards, > Jonas Thank you for pointing this out, I actually missed that. I have added this in v2 of the patch. Kind regards, Marcel
