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;
Date:   Sun, 19 Jan 2020 11:00:31 +0100 (CET)
Message-Id: <20200119.110031.253679721520131241.davem@davemloft.net>
To:     blackgod016574@gmail.com
Cc:     siva.kallam@broadcom.com, prashant@broadcom.com,
        mchan@broadcom.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] driver: tg3: fix potential UAF in
 tigon3_dma_hwbug_workaround()
From:   David Miller <davem@davemloft.net>
In-Reply-To: <20200116033044.GA2783@hunterzg-yangtiant6900c-00>
References: <20200116033044.GA2783@hunterzg-yangtiant6900c-00>
Mime-Version: 1.0
Content-Type: Text/Plain; charset=us-ascii
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

From: Gen Zhang <blackgod016574@gmail.com>
Date: Thu, 16 Jan 2020 11:30:44 +0800

> In tigon3_dma_hwbug_workaround(), pskb is first stored in skb. And this
> function is to store new_skb into pskb at the end. However, in the error
> paths when new_skb is freed by dev_kfree_skb_any(), stroing new_skb to pskb
> should be prevented.
> 
> And freeing skb with dev_consume_skb_any() should be executed after storing
> new_skb to pskb, because freeing skb will free pskb (alias).
> 
> Signed-off-by: Gen Zhang <blackgod016574@gmail.com>

There are no bugs here.

The caller never references "*pskb" when an error is returned.  So it is
safe to store any value whatsoever into that pointer.

'skb' never changes it's value even if we store something into *pskb
because we've loaded it into a local variable.  So it is always safe to
call dev_consume_skb_any() on 'skb' in any order with respect to that
assignment.

I'm not applying this until you can show a real bug resulting from
the current code, and if so you'll need to add that explanation to
your commit message.

Thanks.