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 <[email protected]>
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
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 <[email protected]>
> 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 <[email protected]>
Tested-by: Vladimir Oltean <[email protected]>
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 <[email protected]>
> Fixes: 146d442c2357 ("net: dsa: Keep a pointer to the skb clone for TX timestamping")
Reviewed-by: Florian Fainelli <[email protected]>
--
Florian
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 <[email protected]>
> > 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?
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 <[email protected]>
> > > 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.
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 <[email protected]>
> > > > 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.
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 <[email protected]>
> 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