* [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
@ 2020-06-16 16:01 Taehee Yoo
2020-06-16 16:16 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Taehee Yoo @ 2020-06-16 16:01 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: ap420073, pshelar, eric.dumazet
In the datapath, the ip_tunnel_lookup() is used and it internally uses
fallback tunnel device pointer, which is fb_tunnel_dev.
This pointer variable should be set to NULL when a fb interface is deleted.
But there is no routine to set fb_tunnel_dev pointer to NULL.
So, this pointer will be still used after interface is deleted and
it eventually results in the use-after-free problem.
Test commands:
ip netns add A
ip netns add B
ip link add eth0 type veth peer name eth1
ip link set eth0 netns A
ip link set eth1 netns B
ip netns exec A ip link set lo up
ip netns exec A ip link set eth0 up
ip netns exec A ip link add gre1 type gre local 10.0.0.1 \
remote 10.0.0.2
ip netns exec A ip link set gre1 up
ip netns exec A ip a a 10.0.100.1/24 dev gre1
ip netns exec A ip a a 10.0.0.1/24 dev eth0
ip netns exec B ip link set lo up
ip netns exec B ip link set eth1 up
ip netns exec B ip link add gre1 type gre local 10.0.0.2 \
remote 10.0.0.1
ip netns exec B ip link set gre1 up
ip netns exec B ip a a 10.0.100.2/24 dev gre1
ip netns exec B ip a a 10.0.0.2/24 dev eth1
ip netns exec A hping3 10.0.100.2 -2 --flood -d 60000 &
ip netns del B
Splat looks like:
[ 133.319668][ C3] BUG: KASAN: use-after-free in ip_tunnel_lookup+0x9d6/0xde0
[ 133.343852][ C3] Read of size 4 at addr ffff8880b1701c84 by task hping3/1222
[ 133.344724][ C3]
[ 133.345002][ C3] CPU: 3 PID: 1222 Comm: hping3 Not tainted 5.7.0+ #591
[ 133.345814][ C3] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[ 133.373336][ C3] Call Trace:
[ 133.374792][ C3] <IRQ>
[ 133.375205][ C3] dump_stack+0x96/0xdb
[ 133.375789][ C3] print_address_description.constprop.6+0x2cc/0x450
[ 133.376720][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
[ 133.377431][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
[ 133.378130][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
[ 133.378851][ C3] kasan_report+0x154/0x190
[ 133.379494][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
[ 133.380200][ C3] ip_tunnel_lookup+0x9d6/0xde0
[ 133.380894][ C3] __ipgre_rcv+0x1ab/0xaa0 [ip_gre]
[ 133.381630][ C3] ? rcu_read_lock_sched_held+0xc0/0xc0
[ 133.382429][ C3] gre_rcv+0x304/0x1910 [ip_gre]
[ ... ]
Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
v1 -> v2:
- Do not add a new variable.
net/ipv4/ip_tunnel.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index f4f1d11eab50..701f150f11e1 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -85,9 +85,10 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
__be32 remote, __be32 local,
__be32 key)
{
- unsigned int hash;
struct ip_tunnel *t, *cand = NULL;
struct hlist_head *head;
+ struct net_device *ndev;
+ unsigned int hash;
hash = ip_tunnel_hash(key, remote);
head = &itn->tunnels[hash];
@@ -162,8 +163,9 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
if (t && t->dev->flags & IFF_UP)
return t;
- if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
- return netdev_priv(itn->fb_tunnel_dev);
+ ndev = READ_ONCE(itn->fb_tunnel_dev);
+ if (ndev && ndev->flags & IFF_UP)
+ return netdev_priv(ndev);
return NULL;
}
@@ -1260,8 +1262,9 @@ void ip_tunnel_uninit(struct net_device *dev)
itn = net_generic(net, tunnel->ip_tnl_net_id);
/* fb_tunnel_dev will be unregisted in net-exit call. */
- if (itn->fb_tunnel_dev != dev)
- ip_tunnel_del(itn, netdev_priv(dev));
+ ip_tunnel_del(itn, netdev_priv(dev));
+ if (itn->fb_tunnel_dev == dev)
+ WRITE_ONCE(itn->fb_tunnel_dev, NULL);
dst_cache_reset(&tunnel->dst_cache);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
2020-06-16 16:01 [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup() Taehee Yoo
@ 2020-06-16 16:16 ` Eric Dumazet
2020-06-16 16:37 ` Taehee Yoo
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2020-06-16 16:16 UTC (permalink / raw)
To: Taehee Yoo, davem, kuba, netdev; +Cc: pshelar, eric.dumazet
On 6/16/20 9:01 AM, Taehee Yoo wrote:
> In the datapath, the ip_tunnel_lookup() is used and it internally uses
> fallback tunnel device pointer, which is fb_tunnel_dev.
> This pointer variable should be set to NULL when a fb interface is deleted.
> But there is no routine to set fb_tunnel_dev pointer to NULL.
> So, this pointer will be still used after interface is deleted and
> it eventually results in the use-after-free problem.
>
> Test commands:
> ip netns add A
> ip netns add B
> ip link add eth0 type veth peer name eth1
> ip link set eth0 netns A
> ip link set eth1 netns B
>
> ip netns exec A ip link set lo up
> ip netns exec A ip link set eth0 up
> ip netns exec A ip link add gre1 type gre local 10.0.0.1 \
> remote 10.0.0.2
> ip netns exec A ip link set gre1 up
> ip netns exec A ip a a 10.0.100.1/24 dev gre1
> ip netns exec A ip a a 10.0.0.1/24 dev eth0
>
> ip netns exec B ip link set lo up
> ip netns exec B ip link set eth1 up
> ip netns exec B ip link add gre1 type gre local 10.0.0.2 \
> remote 10.0.0.1
> ip netns exec B ip link set gre1 up
> ip netns exec B ip a a 10.0.100.2/24 dev gre1
> ip netns exec B ip a a 10.0.0.2/24 dev eth1
> ip netns exec A hping3 10.0.100.2 -2 --flood -d 60000 &
> ip netns del B
>
> Splat looks like:
> [ 133.319668][ C3] BUG: KASAN: use-after-free in ip_tunnel_lookup+0x9d6/0xde0
> [ 133.343852][ C3] Read of size 4 at addr ffff8880b1701c84 by task hping3/1222
> [ 133.344724][ C3]
> [ 133.345002][ C3] CPU: 3 PID: 1222 Comm: hping3 Not tainted 5.7.0+ #591
> [ 133.345814][ C3] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> [ 133.373336][ C3] Call Trace:
> [ 133.374792][ C3] <IRQ>
> [ 133.375205][ C3] dump_stack+0x96/0xdb
> [ 133.375789][ C3] print_address_description.constprop.6+0x2cc/0x450
> [ 133.376720][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> [ 133.377431][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> [ 133.378130][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> [ 133.378851][ C3] kasan_report+0x154/0x190
> [ 133.379494][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> [ 133.380200][ C3] ip_tunnel_lookup+0x9d6/0xde0
> [ 133.380894][ C3] __ipgre_rcv+0x1ab/0xaa0 [ip_gre]
> [ 133.381630][ C3] ? rcu_read_lock_sched_held+0xc0/0xc0
> [ 133.382429][ C3] gre_rcv+0x304/0x1910 [ip_gre]
> [ ... ]
>
> Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
>
> v1 -> v2:
> - Do not add a new variable.
>
> net/ipv4/ip_tunnel.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index f4f1d11eab50..701f150f11e1 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -85,9 +85,10 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> __be32 remote, __be32 local,
> __be32 key)
> {
> - unsigned int hash;
> struct ip_tunnel *t, *cand = NULL;
> struct hlist_head *head;
> + struct net_device *ndev;
> + unsigned int hash;
>
> hash = ip_tunnel_hash(key, remote);
> head = &itn->tunnels[hash];
> @@ -162,8 +163,9 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> if (t && t->dev->flags & IFF_UP)
> return t;
>
> - if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
> - return netdev_priv(itn->fb_tunnel_dev);
> + ndev = READ_ONCE(itn->fb_tunnel_dev);
> + if (ndev && ndev->flags & IFF_UP)
> + return netdev_priv(ndev);
>
> return NULL;
> }
> @@ -1260,8 +1262,9 @@ void ip_tunnel_uninit(struct net_device *dev)
>
> itn = net_generic(net, tunnel->ip_tnl_net_id);
> /* fb_tunnel_dev will be unregisted in net-exit call. */
Is this comment still correct ?
> - if (itn->fb_tunnel_dev != dev)
> - ip_tunnel_del(itn, netdev_priv(dev));
> + ip_tunnel_del(itn, netdev_priv(dev));
> + if (itn->fb_tunnel_dev == dev)
> + WRITE_ONCE(itn->fb_tunnel_dev, NULL);
>
> dst_cache_reset(&tunnel->dst_cache);
> }
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
2020-06-16 16:16 ` Eric Dumazet
@ 2020-06-16 16:37 ` Taehee Yoo
0 siblings, 0 replies; 3+ messages in thread
From: Taehee Yoo @ 2020-06-16 16:37 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Jakub Kicinski, Netdev, pshelar
On Wed, 17 Jun 2020 at 01:16, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
Hi Eric,
Thank you for the review :)
>
> On 6/16/20 9:01 AM, Taehee Yoo wrote:
> > In the datapath, the ip_tunnel_lookup() is used and it internally uses
> > fallback tunnel device pointer, which is fb_tunnel_dev.
> > This pointer variable should be set to NULL when a fb interface is deleted.
> > But there is no routine to set fb_tunnel_dev pointer to NULL.
> > So, this pointer will be still used after interface is deleted and
> > it eventually results in the use-after-free problem.
> >
> > Test commands:
> > ip netns add A
> > ip netns add B
> > ip link add eth0 type veth peer name eth1
> > ip link set eth0 netns A
> > ip link set eth1 netns B
> >
> > ip netns exec A ip link set lo up
> > ip netns exec A ip link set eth0 up
> > ip netns exec A ip link add gre1 type gre local 10.0.0.1 \
> > remote 10.0.0.2
> > ip netns exec A ip link set gre1 up
> > ip netns exec A ip a a 10.0.100.1/24 dev gre1
> > ip netns exec A ip a a 10.0.0.1/24 dev eth0
> >
> > ip netns exec B ip link set lo up
> > ip netns exec B ip link set eth1 up
> > ip netns exec B ip link add gre1 type gre local 10.0.0.2 \
> > remote 10.0.0.1
> > ip netns exec B ip link set gre1 up
> > ip netns exec B ip a a 10.0.100.2/24 dev gre1
> > ip netns exec B ip a a 10.0.0.2/24 dev eth1
> > ip netns exec A hping3 10.0.100.2 -2 --flood -d 60000 &
> > ip netns del B
> >
> > Splat looks like:
> > [ 133.319668][ C3] BUG: KASAN: use-after-free in ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.343852][ C3] Read of size 4 at addr ffff8880b1701c84 by task hping3/1222
> > [ 133.344724][ C3]
> > [ 133.345002][ C3] CPU: 3 PID: 1222 Comm: hping3 Not tainted 5.7.0+ #591
> > [ 133.345814][ C3] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
> > [ 133.373336][ C3] Call Trace:
> > [ 133.374792][ C3] <IRQ>
> > [ 133.375205][ C3] dump_stack+0x96/0xdb
> > [ 133.375789][ C3] print_address_description.constprop.6+0x2cc/0x450
> > [ 133.376720][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.377431][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.378130][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.378851][ C3] kasan_report+0x154/0x190
> > [ 133.379494][ C3] ? ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.380200][ C3] ip_tunnel_lookup+0x9d6/0xde0
> > [ 133.380894][ C3] __ipgre_rcv+0x1ab/0xaa0 [ip_gre]
> > [ 133.381630][ C3] ? rcu_read_lock_sched_held+0xc0/0xc0
> > [ 133.382429][ C3] gre_rcv+0x304/0x1910 [ip_gre]
> > [ ... ]
> >
> > Suggested-by: Eric Dumazet <eric.dumazet@gmail.com>
> > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> >
> > v1 -> v2:
> > - Do not add a new variable.
> >
> > net/ipv4/ip_tunnel.c | 13 ++++++++-----
> > 1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> > index f4f1d11eab50..701f150f11e1 100644
> > --- a/net/ipv4/ip_tunnel.c
> > +++ b/net/ipv4/ip_tunnel.c
> > @@ -85,9 +85,10 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> > __be32 remote, __be32 local,
> > __be32 key)
> > {
> > - unsigned int hash;
> > struct ip_tunnel *t, *cand = NULL;
> > struct hlist_head *head;
> > + struct net_device *ndev;
> > + unsigned int hash;
> >
> > hash = ip_tunnel_hash(key, remote);
> > head = &itn->tunnels[hash];
> > @@ -162,8 +163,9 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> > if (t && t->dev->flags & IFF_UP)
> > return t;
> >
> > - if (itn->fb_tunnel_dev && itn->fb_tunnel_dev->flags & IFF_UP)
> > - return netdev_priv(itn->fb_tunnel_dev);
> > + ndev = READ_ONCE(itn->fb_tunnel_dev);
> > + if (ndev && ndev->flags & IFF_UP)
> > + return netdev_priv(ndev);
> >
> > return NULL;
> > }
> > @@ -1260,8 +1262,9 @@ void ip_tunnel_uninit(struct net_device *dev)
> >
> > itn = net_generic(net, tunnel->ip_tnl_net_id);
> > /* fb_tunnel_dev will be unregisted in net-exit call. */
>
> Is this comment still correct ?
>
I think this comment is not valid anymore.
I will remove this comment in a v3 patch.
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-16 16:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-16 16:01 [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup() Taehee Yoo
2020-06-16 16:16 ` Eric Dumazet
2020-06-16 16:37 ` Taehee Yoo
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).