| Message ID | 20201119110906.25558-1-ceggers@arri.de |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v2] net: dsa: avoid potential use-after-free error | expand |
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>
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
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?
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.
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.
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
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); }
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(-)