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

[Andy Gospodarek <andrew.gospodarek@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 2511 bytes --]

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@kernel.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@broadcom.com>

> ---
> CC: michael.chan@broadcom.com
> CC: pavan.chebbi@broadcom.com
> CC: edwin.peer@broadcom.com
> CC: andrew.gospodarek@broadcom.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
> 

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]