[PATCH net-next v2] net: dsa: avoid potential use-after-free error

[PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Christian Eggers]

If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
immediately. Shouldn't store a pointer to freed memory.

Signed-off-by: Christian Eggers <ceggers@arri.de>
Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
---
Changes since v1:
- Fixed "Fixes:" tag (and configured my GIT)
- Adjusted commit description

 net/dsa/slave.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ff2266d2b998..7efc753e4d9d 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -522,10 +522,10 @@ static void dsa_skb_tx_timestamp(struct dsa_slave_priv *p,
 	if (!clone)
 		return;
 
-	DSA_SKB_CB(skb)->clone = clone;
-
-	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type))
+	if (ds->ops->port_txtstamp(ds, p->dp->index, clone, type)) {
+		DSA_SKB_CB(skb)->clone = clone;
 		return;
+	}
 
 	kfree_skb(clone);
 }
-- 
Christian Eggers
Embedded software developer

Arnold & Richter Cine Technik GmbH & Co. Betriebs KG
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRA 57918
Persoenlich haftender Gesellschafter: Arnold & Richter Cine Technik GmbH
Sitz: Muenchen - Registergericht: Amtsgericht Muenchen - Handelsregisternummer: HRB 54477
Geschaeftsfuehrer: Dr. Michael Neuhaeuser; Stephan Schenk; Walter Trauninger; Markus Zeiler

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Vladimir Oltean]

On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:
> If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> immediately. Shouldn't store a pointer to freed memory.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
> ---

IMO this is one of the cases to which the following from
Documentation/process/stable-kernel-rules.rst does not apply:

 - It must fix a real bug that bothers people (not a, "This could be a
   problem..." type thing).

Therefore, specifying "net-next" as the target tree here as opposed to
"net" is the correct choice.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
Tested-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Jakub Kicinski]

On Fri, 20 Nov 2020 20:01:49 +0200 Vladimir Oltean wrote:
> On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:
> > If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> > immediately. Shouldn't store a pointer to freed memory.
> > 
> > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
> > ---  
> 
> IMO this is one of the cases to which the following from
> Documentation/process/stable-kernel-rules.rst does not apply:
> 
>  - It must fix a real bug that bothers people (not a, "This could be a
>    problem..." type thing).
> 
> Therefore, specifying "net-next" as the target tree here as opposed to
> "net" is the correct choice.

The commit message doesn't really explain what happens after.

Is the dangling pointer ever accessed?

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Vladimir Oltean]

On Fri, Nov 20, 2020 at 12:59:21PM -0800, Jakub Kicinski wrote:
> On Fri, 20 Nov 2020 20:01:49 +0200 Vladimir Oltean wrote:
> > On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:
> > > If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> > > immediately. Shouldn't store a pointer to freed memory.
> > >
> > > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > > Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
> > > ---
> >
> > IMO this is one of the cases to which the following from
> > Documentation/process/stable-kernel-rules.rst does not apply:
> >
> >  - It must fix a real bug that bothers people (not a, "This could be a
> >    problem..." type thing).
> >
> > Therefore, specifying "net-next" as the target tree here as opposed to
> > "net" is the correct choice.
>
> The commit message doesn't really explain what happens after.
>
> Is the dangling pointer ever accessed?

Nothing happens afterwards. He explained that he accessed it once while
working on his ksz9477 PTP series. There's no code affected by this in
mainline.

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Jakub Kicinski]

On Fri, 20 Nov 2020 23:04:36 +0200 Vladimir Oltean wrote:
> On Fri, Nov 20, 2020 at 12:59:21PM -0800, Jakub Kicinski wrote:
> > On Fri, 20 Nov 2020 20:01:49 +0200 Vladimir Oltean wrote:  
> > > On Thu, Nov 19, 2020 at 12:09:06PM +0100, Christian Eggers wrote:  
> > > > If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> > > > immediately. Shouldn't store a pointer to freed memory.
> > > >
> > > > Signed-off-by: Christian Eggers <ceggers@arri.de>
> > > > Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
> > > > ---  
> > >
> > > IMO this is one of the cases to which the following from
> > > Documentation/process/stable-kernel-rules.rst does not apply:
> > >
> > >  - It must fix a real bug that bothers people (not a, "This could be a
> > >    problem..." type thing).
> > >
> > > Therefore, specifying "net-next" as the target tree here as opposed to
> > > "net" is the correct choice.  
> >
> > The commit message doesn't really explain what happens after.
> >
> > Is the dangling pointer ever accessed?  
> 
> Nothing happens afterwards. He explained that he accessed it once while
> working on his ksz9477 PTP series. There's no code affected by this in
> mainline.

Ah, great, I'll drop the Fixes tag altogether then.

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[Florian Fainelli]

On 11/19/20 3:09 AM, Christian Eggers wrote:
> If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> immediately. Shouldn't store a pointer to freed memory.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (refs/heads/master):

On Thu, 19 Nov 2020 12:09:06 +0100 you wrote:
> If dsa_switch_ops::port_txtstamp() returns false, clone will be freed
> immediately. Shouldn't store a pointer to freed memory.
> 
> Signed-off-by: Christian Eggers <ceggers@arri.de>
> Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
> ---
> Changes since v1:
> - Fixed "Fixes:" tag (and configured my GIT)
> - Adjusted commit description
> 
> [...]

Here is the summary with links:
  - [net-next,v2] net: dsa: avoid potential use-after-free error
    https://git.kernel.org/netdev/net-next/c/30abc9cd9c6b

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