netdev - Re: [PATCH net] bnxt: prevent skb UAF after handing over to PTP worker

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YytzV3H+fSqBbxTh@C02YVCJELVCG>
Date:   Wed, 21 Sep 2022 16:25:59 -0400
From:   Andy Gospodarek <andrew.gospodarek@...adcom.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     davem@...emloft.net, netdev@...r.kernel.org, edumazet@...gle.com,
        pabeni@...hat.com, michael.chan@...adcom.com,
        pavan.chebbi@...adcom.com, andrew.gospodarek@...adcom.com
Subject: Re: [PATCH net] bnxt: prevent skb UAF after handing over to PTP
 worker

On Wed, Sep 21, 2022 at 01:10:05PM -0700, Jakub Kicinski wrote:
> When reading the timestamp is required bnxt_tx_int() hands
> over the ownership of the completed skb to the PTP worker.
> The skb should not be used afterwards, as the worker may
> run before the rest of our code and free the skb, leading
> to a use-after-free.
> 
> Since dev_kfree_skb_any() accepts NULL make the loss of
> ownership more obvious and set skb to NULL.
> 
> Fixes: 83bb623c968e ("bnxt_en: Transmit and retrieve packet timestamps")
> Signed-off-by: Jakub Kicinski <kuba@...nel.org>

In general this looks good to me.  Let's make sure Pavan and Michael
also agree.  Thanks for the patch!

Reviewed-by: Andy Gospodarek <gospo@...adcom.com>

> ---
> CC: michael.chan@...adcom.com
> CC: pavan.chebbi@...adcom.com
> CC: edwin.peer@...adcom.com
> CC: andrew.gospodarek@...adcom.com
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index f46eefb5a029..96da0ba3d507 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -659,7 +659,6 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>  
>  	for (i = 0; i < nr_pkts; i++) {
>  		struct bnxt_sw_tx_bd *tx_buf;
> -		bool compl_deferred = false;
>  		struct sk_buff *skb;
>  		int j, last;
>  
> @@ -668,6 +667,8 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>  		skb = tx_buf->skb;
>  		tx_buf->skb = NULL;
>  
> +		tx_bytes += skb->len;
> +
>  		if (tx_buf->is_push) {
>  			tx_buf->is_push = 0;
>  			goto next_tx_int;
> @@ -688,8 +689,9 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>  		}
>  		if (unlikely(skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS)) {
>  			if (bp->flags & BNXT_FLAG_CHIP_P5) {
> +				/* PTP worker takes ownership of the skb */
>  				if (!bnxt_get_tx_ts_p5(bp, skb))
> -					compl_deferred = true;
> +					skb = NULL;
>  				else
>  					atomic_inc(&bp->ptp_cfg->tx_avail);
>  			}
> @@ -698,9 +700,7 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
>  next_tx_int:
>  		cons = NEXT_TX(cons);
>  
> -		tx_bytes += skb->len;
> -		if (!compl_deferred)
> -			dev_kfree_skb_any(skb);
> +		dev_kfree_skb_any(skb);
>  	}
>  
>  	netdev_tx_completed_queue(txq, nr_pkts, tx_bytes);
> -- 
> 2.37.3
> 

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4222 bytes)