linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next v2] net: dsa: avoid potential use-after-free error
@ 2020-11-19 11:09 Christian Eggers
  2020-11-20 18:01 ` Vladimir Oltean
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Christian Eggers @ 2020-11-19 11:09 UTC (permalink / raw)
  To: Andrew Lunn, Florian Fainelli, Vladimir Oltean
  Cc: Vivien Didelot, David S . Miller, Jakub Kicinski, netdev,
	linux-kernel, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-19 11:09 [PATCH net-next v2] net: dsa: avoid potential use-after-free error Christian Eggers
@ 2020-11-20 18:01 ` Vladimir Oltean
  2020-11-20 20:59   ` Jakub Kicinski
  2020-11-20 18:17 ` Florian Fainelli
  2020-11-20 23:10 ` patchwork-bot+netdevbpf
  2 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2020-11-20 18:01 UTC (permalink / raw)
  To: Christian Eggers
  Cc: Andrew Lunn, Florian Fainelli, Vivien Didelot, David S . Miller,
	Jakub Kicinski, netdev, linux-kernel

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>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-19 11:09 [PATCH net-next v2] net: dsa: avoid potential use-after-free error Christian Eggers
  2020-11-20 18:01 ` Vladimir Oltean
@ 2020-11-20 18:17 ` Florian Fainelli
  2020-11-20 23:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 7+ messages in thread
From: Florian Fainelli @ 2020-11-20 18:17 UTC (permalink / raw)
  To: Christian Eggers, Andrew Lunn, Vladimir Oltean
  Cc: Vivien Didelot, David S . Miller, Jakub Kicinski, netdev, linux-kernel

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-20 18:01 ` Vladimir Oltean
@ 2020-11-20 20:59   ` Jakub Kicinski
  2020-11-20 21:04     ` Vladimir Oltean
  0 siblings, 1 reply; 7+ messages in thread
From: Jakub Kicinski @ 2020-11-20 20:59 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Christian Eggers, Andrew Lunn, Florian Fainelli, Vivien Didelot,
	David S . Miller, netdev, linux-kernel

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?

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-20 20:59   ` Jakub Kicinski
@ 2020-11-20 21:04     ` Vladimir Oltean
  2020-11-20 21:17       ` Jakub Kicinski
  0 siblings, 1 reply; 7+ messages in thread
From: Vladimir Oltean @ 2020-11-20 21:04 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Christian Eggers, Andrew Lunn, Florian Fainelli, Vivien Didelot,
	David S . Miller, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-20 21:04     ` Vladimir Oltean
@ 2020-11-20 21:17       ` Jakub Kicinski
  0 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2020-11-20 21:17 UTC (permalink / raw)
  To: Vladimir Oltean
  Cc: Christian Eggers, Andrew Lunn, Florian Fainelli, Vivien Didelot,
	David S . Miller, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH net-next v2] net: dsa: avoid potential use-after-free error
  2020-11-19 11:09 [PATCH net-next v2] net: dsa: avoid potential use-after-free error Christian Eggers
  2020-11-20 18:01 ` Vladimir Oltean
  2020-11-20 18:17 ` Florian Fainelli
@ 2020-11-20 23:10 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 7+ messages in thread
From: patchwork-bot+netdevbpf @ 2020-11-20 23:10 UTC (permalink / raw)
  To: Christian Eggers
  Cc: andrew, f.fainelli, olteanv, vivien.didelot, davem, kuba, netdev,
	linux-kernel

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



^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-11-20 23:10 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-19 11:09 [PATCH net-next v2] net: dsa: avoid potential use-after-free error Christian Eggers
2020-11-20 18:01 ` Vladimir Oltean
2020-11-20 20:59   ` Jakub Kicinski
2020-11-20 21:04     ` Vladimir Oltean
2020-11-20 21:17       ` Jakub Kicinski
2020-11-20 18:17 ` Florian Fainelli
2020-11-20 23:10 ` patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).