brcmfmac: fix use-after-free bug

Signed-off-by: Alexander Coffin <alex.coffin@matician.com>
---
 drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

On 8/2/2022 7:28 PM, Alexander Coffin wrote:
 >

A commit message would have been nice...

> Signed-off-by: Alexander Coffin <alex.coffin@matician.com>
> ---
>   drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> index 87aef211b35f..12ee8b7163fd 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
> @@ -296,6 +296,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>   	struct brcmf_pub *drvr = ifp->drvr;
>   	struct ethhdr *eh;
>   	int head_delta;
> +	unsigned int tx_bytes = skb->len;
>   
>   	brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
>   
> @@ -370,7 +371,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct sk_buff *skb,
>   		ndev->stats.tx_dropped++;
>   	} else {
>   		ndev->stats.tx_packets++;
> -		ndev->stats.tx_bytes += skb->len;
> +		ndev->stats.tx_bytes += tx_bytes;

Why would this be a use-after-free? We only get here when ret is zero. 
In that case the skb is not freed. If there would be a commit message 
with some error report that proofs there is a use-after-free I would 
look into this further, but now I just say NAK.

Regards,
Arend

>   	}
>   
>   	/* Return ok: we always eat the packet */

I am resending this message as apparently it wasn't delivered to some
people as it was an HTML message. I apologize for the double email,
but I forgot to tell Gmail to use plain text only.

Arend,

> A commit message would have been nice...

> If there would be a commit message with some error report that proofs there is a use-after-free

I apologize for not including a longer commit message. I thought that
my stack trace in my first email would be sufficient, but looking back
I see how I should have clarified what was going wrong. What occurs is
that line 360 of core.c

> ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);

may be entirely completed (as in not only scheduled, but also the
entire transaction may have completed) by the time that `skb->len` is
invoked which means that skb will have been freed by the corresponding
later function (in this case brcmu_pkt_buf_free_skb if you see the
trace from my first email).

> We only get here when ret is zero.

Therefore this error only occurs when ret is zero, but skb may have
been freed after line 360, and before that line (369) if how the
kernel schedules tasks is very unfavorable.

> ndev->stats.tx_bytes += skb->len;

Please let me know if you need any further information.

Sorry,
Alexander Coffin

On 8/8/2022 11:38 AM, Alexander Coffin wrote:
> I am resending this message as apparently it wasn't delivered to some
> people as it was an HTML message. I apologize for the double email,
> but I forgot to tell Gmail to use plain text only.
> 
> Arend,
> 
>> A commit message would have been nice...
> 
>> If there would be a commit message with some error report that proofs there is a use-after-free
> 
> I apologize for not including a longer commit message. I thought that
> my stack trace in my first email would be sufficient, but looking back
> I see how I should have clarified what was going wrong. What occurs is
> that line 360 of core.c
> 
>> ret = brcmf_proto_tx_queue_data(drvr, ifp->ifidx, skb);
> 
> may be entirely completed (as in not only scheduled, but also the
> entire transaction may have completed) by the time that `skb->len` is
> invoked which means that skb will have been freed by the corresponding
> later function (in this case brcmu_pkt_buf_free_skb if you see the
> trace from my first email).
> 
>> We only get here when ret is zero.
> 
> Therefore this error only occurs when ret is zero, but skb may have
> been freed after line 360, and before that line (369) if how the
> kernel schedules tasks is very unfavorable.
> 
>> ndev->stats.tx_bytes += skb->len;
> 
> Please let me know if you need any further information.

Not sure I've seen your first email. It is better to have it all in the 
patch so it is clear the patch solves an actual observed failure. So if 
you can dig up that stack trace and add it to the patch I will be happy 
to acknowledge the patch so it can be taken upstream.

Regards,
Arend