| Message ID | 20200616160153.8479-1-ap420073@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net,v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup() | expand |
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); > } >
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
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); }
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(-)