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@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/ --- 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); }
- if (!success) + if (!success && ifp) ifp->ndev->stats.tx_errors++;
brcmu_pkt_buf_free_skb(txp);
Hi,
On Thu, Jan 9, 2025 at 4:53 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.
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/
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
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@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.
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/
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
linux-stable-mirror@lists.linaro.org
-
Jonas Gorski -
Marcel Hamer