diff mbox series

[v2] brcmfmac: NULL pointer dereference on tx statistic update

Message ID 20250110134502.824722-1-marcel.hamer@windriver.com
State New
Headers show
Series [v2] brcmfmac: NULL pointer dereference on tx statistic update | expand

Commit Message

Marcel Hamer Jan. 10, 2025, 1:45 p.m. UTC 
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.

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/
---
v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Arend van Spriel Jan. 13, 2025, 7:28 p.m. UTC | #1 
On 1/10/2025 2:45 PM, Marcel Hamer 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.

Here my suggestion to make it a bit more simple...

> 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/
> ---
> v1 -> v2: guard all uses of the ifp pointer inside brcmf_txfinalize()
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index c3a57e30c855..791757a3ec13 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -543,13 +543,13 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)

Instead of checking ifp below you can simply do following here and be 
done with it:

	if (!ifp) {
		brcmu_pkt_buf_free_skb(txp);
		return;
	}

>   	eh = (struct ethhdr *)(txp->data);
>   	type = ntohs(eh->h_proto);
>   
> -	if (type == ETH_P_PAE) {
> +	if (type == ETH_P_PAE && ifp) {
>   		atomic_dec(&ifp->pend_8021x_cnt);
>   		if (waitqueue_active(&ifp->pend_8021x_wait))
>   			wake_up(&ifp->pend_8021x_wait);
>   	}
>   
> -	if (!success)
> +	if (!success && ifp)
>   		ifp->ndev->stats.tx_errors++;
>   
>   	brcmu_pkt_buf_free_skb(txp);
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index c3a57e30c855..791757a3ec13 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -543,13 +543,13 @@  void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
 	eh = (struct ethhdr *)(txp->data);
 	type = ntohs(eh->h_proto);
 
-	if (type == ETH_P_PAE) {
+	if (type == ETH_P_PAE && ifp) {
 		atomic_dec(&ifp->pend_8021x_cnt);
 		if (waitqueue_active(&ifp->pend_8021x_wait))
 			wake_up(&ifp->pend_8021x_wait);
 	}
 
-	if (!success)
+	if (!success && ifp)
 		ifp->ndev->stats.tx_errors++;
 
 	brcmu_pkt_buf_free_skb(txp);