Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] net: nvidia: forcedeth: Fix two possible concurrency
 use-after-free bugs
To:     Jia-Ju Bai <baijiaju1990@gmail.com>, davem@davemloft.net,
        keescook@chromium.org
Cc:     netdev@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20190108124518.21986-1-baijiaju1990@gmail.com>
From:   Zhu Yanjun <yanjun.zhu@oracle.com>
Message-ID: <27392ae0-2c0f-f099-05d8-f9cdbfbd313e@oracle.com>
Date:   Tue, 8 Jan 2019 20:54:20 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.4.0
MIME-Version: 1.0
In-Reply-To: <20190108124518.21986-1-baijiaju1990@gmail.com>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


?? 2019/1/8 20:45, Jia-Ju Bai д??:
> In drivers/net/ethernet/nvidia/forcedeth.c, the functions
> nv_start_xmit() and nv_start_xmit_optimized() can be concurrently
> executed with nv_poll_controller().
>
> nv_start_xmit
>    line 2321: prev_tx_ctx->skb = skb;
>
> nv_start_xmit_optimized
>    line 2479: prev_tx_ctx->skb = skb;
>
> nv_poll_controller
>    nv_do_nic_poll
>      line 4134: spin_lock(&np->lock);
>      nv_drain_rxtx
>        nv_drain_tx
>          nv_release_txskb
>            line 2004: dev_kfree_skb_any(tx_skb->skb);
>
> Thus, two possible concurrency use-after-free bugs may occur.
>
> To fix these possible bugs,


Does this really occur? Can you reproduce this ?


>   the calls to spin_lock_irqsave() in
> nv_start_xmit() and nv_start_xmit_optimized() are moved to the
> front of "prev_tx_ctx->skb = skb;"
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
> ---
>   drivers/net/ethernet/nvidia/forcedeth.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index 1d9b0d44ddb6..48fa5a0bd2cb 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -2317,6 +2317,8 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	/* set last fragment flag  */
>   	prev_tx->flaglen |= cpu_to_le32(tx_flags_extra);
>   
> +	spin_lock_irqsave(&np->lock, flags);
> +
>   	/* save skb in this slot's context area */
>   	prev_tx_ctx->skb = skb;
>   
> @@ -2326,8 +2328,6 @@ static netdev_tx_t nv_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   		tx_flags_extra = skb->ip_summed == CHECKSUM_PARTIAL ?
>   			 NV_TX2_CHECKSUM_L3 | NV_TX2_CHECKSUM_L4 : 0;
>   
> -	spin_lock_irqsave(&np->lock, flags);
> -
>   	/* set tx flags */
>   	start_tx->flaglen |= cpu_to_le32(tx_flags | tx_flags_extra);
>   
> @@ -2475,6 +2475,8 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
>   	/* set last fragment flag  */
>   	prev_tx->flaglen |= cpu_to_le32(NV_TX2_LASTPACKET);
>   
> +	spin_lock_irqsave(&np->lock, flags);
> +
>   	/* save skb in this slot's context area */
>   	prev_tx_ctx->skb = skb;
>   
> @@ -2491,8 +2493,6 @@ static netdev_tx_t nv_start_xmit_optimized(struct sk_buff *skb,
>   	else
>   		start_tx->txvlan = 0;
>   
> -	spin_lock_irqsave(&np->lock, flags);
> -
>   	if (np->tx_limit) {
>   		/* Limit the number of outstanding tx. Setup all fragments, but
>   		 * do not set the VALID bit on the first descriptor. Save a pointer