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

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

[Jakub Kicinski]

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>
---
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

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

[Andy Gospodarek]

[-- 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 --]

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

[Michael Chan]

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

On Wed, Sep 21, 2022 at 1:26 PM Andy Gospodarek
<andrew.gospodarek@broadcom.com> wrote:
>
> 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>

Thanks for catching this.

Reviewed-by: Michael Chan <michael.chan@broadcom.com>

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

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

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 21 Sep 2022 13:10:05 -0700 you 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.
> 
> [...]

Here is the summary with links:
  - [net] bnxt: prevent skb UAF after handing over to PTP worker
    https://git.kernel.org/netdev/net/c/c31f26c8f69f

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html