| Message ID | 20250116132240.731039-1-marcel.hamer@windriver.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 68abd0c4ebf24cd499841a488b97a6873d5efabb |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v3] wifi: brcmfmac: fix NULL pointer dereference in brcmf_txfinalize() | expand |
On January 16, 2025 2:22:55 PM Marcel Hamer <marcel.hamer@windriver.com> 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. > > Although the NULL pointer dereference has only been seen when trying to > update the tx statistic, all other uses of the ifp pointer have been > guarded as well with an early return if ifp is NULL. > > Cc: stable@vger.kernel.org Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com> > Link: > https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/
Marcel Hamer <marcel.hamer@windriver.com> 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. > > Although the NULL pointer dereference has only been seen when trying to > update the tx statistic, all other uses of the ifp pointer have been > guarded as well with an early return if ifp is NULL. > > Cc: stable@vger.kernel.org > Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com> > Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/ > Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> Patch applied to wireless-next.git, thanks. 68abd0c4ebf2 wifi: brcmfmac: fix NULL pointer dereference in brcmf_txfinalize()
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c index c3a57e30c855..3d63010ae079 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c @@ -540,6 +540,11 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success) struct ethhdr *eh; u16 type; + if (!ifp) { + brcmu_pkt_buf_free_skb(txp); + return; + } + eh = (struct ethhdr *)(txp->data); type = ntohs(eh->h_proto);
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. Although the NULL pointer dereference has only been seen when trying to update the tx statistic, all other uses of the ifp pointer have been guarded as well with an early return if ifp is NULL. Cc: stable@vger.kernel.org Signed-off-by: Marcel Hamer <marcel.hamer@windriver.com> Link: https://lore.kernel.org/all/b519e746-ddfd-421f-d897-7620d229e4b2@gmail.com/ --- v3: - after review return early from brcmf_txfinalize() if ifp is NULL - after review improve the subject line v2: https://lore.kernel.org/linux-wireless/20250110134502.824722-1-marcel.hamer@windriver.com/ - guard all uses of the ifp pointer inside brcmf_txfinalize() v1: https://lore.kernel.org/linux-wireless/20250109155201.3661298-1-marcel.hamer@windriver.com/ - initial version --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 5 +++++ 1 file changed, 5 insertions(+)