[PATCH] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()

[PATCH] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()

[Wei Yongjun]

From: Wei Yongjun <weiyongjun1@huawei.com>

skb_share_check() already drop the reference of skb when return
NULL, using kfree_skb() in the error handling path lead to skb
double free.

Fix it by remve the variable tmp_skb, and return directly when
skb_share_check() return NULL.

Fixes: 01a4cc4d0cd6 ("bnx2fc: do not add shared skbs to the fcoe_rx_list")
Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
---
 drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
index 05ddbb9bb7d8..451a58e0fd96 100644
--- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
+++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
@@ -429,7 +429,6 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 	struct fcoe_ctlr *ctlr;
 	struct fcoe_rcv_info *fr;
 	struct fcoe_percpu_s *bg;
-	struct sk_buff *tmp_skb;
 
 	interface = container_of(ptype, struct bnx2fc_interface,
 				 fcoe_packet_type);
@@ -441,11 +440,9 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto err;
 	}
 
-	tmp_skb = skb_share_check(skb, GFP_ATOMIC);
-	if (!tmp_skb)
-		goto err;
-
-	skb = tmp_skb;
+	skb = skb_share_check(skb, GFP_ATOMIC);
+	if (!skb)
+		return -1;
 
 	if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
 		printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
-- 
2.34.1

Re: [PATCH] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()

[Dan Carpenter]

What ever happened to this patch?  I was reviewing old use after free
static checker warnings (Smatch) and came across it.  The patch looks
correct to me (I wrote the exact same patch myself before seeing this
one on lore).

regards,
dan carpenter


On Mon, Nov 14, 2022 at 11:06:26AM +0000, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> skb_share_check() already drop the reference of skb when return
> NULL, using kfree_skb() in the error handling path lead to skb
> double free.
> 
> Fix it by remve the variable tmp_skb, and return directly when
> skb_share_check() return NULL.
> 
> Fixes: 01a4cc4d0cd6 ("bnx2fc: do not add shared skbs to the fcoe_rx_list")
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  drivers/scsi/bnx2fc/bnx2fc_fcoe.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> index 05ddbb9bb7d8..451a58e0fd96 100644
> --- a/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_fcoe.c
> @@ -429,7 +429,6 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
>  	struct fcoe_ctlr *ctlr;
>  	struct fcoe_rcv_info *fr;
>  	struct fcoe_percpu_s *bg;
> -	struct sk_buff *tmp_skb;
>  
>  	interface = container_of(ptype, struct bnx2fc_interface,
>  				 fcoe_packet_type);
> @@ -441,11 +440,9 @@ static int bnx2fc_rcv(struct sk_buff *skb, struct net_device *dev,
>  		goto err;
>  	}
>  
> -	tmp_skb = skb_share_check(skb, GFP_ATOMIC);
> -	if (!tmp_skb)
> -		goto err;
> -
> -	skb = tmp_skb;
> +	skb = skb_share_check(skb, GFP_ATOMIC);
> +	if (!skb)
> +		return -1;
>  
>  	if (unlikely(eth_hdr(skb)->h_proto != htons(ETH_P_FCOE))) {
>  		printk(KERN_ERR PFX "bnx2fc_rcv: Wrong FC type frame\n");
> -- 
> 2.34.1
> 

Re: [PATCH] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()

[Martin K. Petersen]

Dan,

> What ever happened to this patch? I was reviewing old use after free
> static checker warnings (Smatch) and came across it. The patch looks
> correct to me (I wrote the exact same patch myself before seeing this
> one on lore).

Not sure what happened. Patchwork had it tagged as "New/archived" which
is really peculiar.

In any case I have applied the patch to 6.7/scsi-fixes, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()

[Martin K. Petersen]

On Mon, 14 Nov 2022 11:06:26 +0000, Wei Yongjun wrote:

> skb_share_check() already drop the reference of skb when return
> NULL, using kfree_skb() in the error handling path lead to skb
> double free.
> 
> Fix it by remve the variable tmp_skb, and return directly when
> skb_share_check() return NULL.
> 
> [...]

Applied to 6.7/scsi-fixes, thanks!

[1/1] scsi: bnx2fc: Fix skb double free in bnx2fc_rcv()
      https://git.kernel.org/mkp/scsi/c/08c94d80b2da

-- 
Martin K. Petersen	Oracle Linux Engineering