* [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
@ 2020-06-15 15:06 Taehee Yoo
2020-06-15 16:02 ` Eric Dumazet
0 siblings, 1 reply; 3+ messages in thread
From: Taehee Yoo @ 2020-06-15 15:06 UTC (permalink / raw)
To: davem, kuba, netdev; +Cc: ap420073, pshelar
In the datapath, the ip_tunnel_lookup() is used and it internally uses
fallback tunnel device pointer, which is fb_tunnel_dev.
This pointer is protected by RTNL. It's not enough to be used
in the datapath.
So, this pointer would be used after an interface is deleted.
It eventually results in the use-after-free problem.
In order to avoid the problem, the new tunnel pointer variable is added,
which indicates a fallback tunnel device's tunnel pointer.
This is protected by both RTNL and RCU.
So, it's safe to be used in the datapath.
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]
[ ... ]
Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
include/net/ip_tunnels.h | 1 +
net/ipv4/ip_tunnel.c | 11 ++++++++---
2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
index 076e5d7db7d3..7442c517bb75 100644
--- a/include/net/ip_tunnels.h
+++ b/include/net/ip_tunnels.h
@@ -164,6 +164,7 @@ struct ip_tunnel_net {
struct rtnl_link_ops *rtnl_link_ops;
struct hlist_head tunnels[IP_TNL_HASH_SIZE];
struct ip_tunnel __rcu *collect_md_tun;
+ struct ip_tunnel __rcu *fb_tun;
int type;
};
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index f4f1d11eab50..285b863e2fcc 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -162,8 +162,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);
+ t = rcu_dereference(itn->fb_tun);
+ if (t && t->dev->flags & IFF_UP)
+ return t;
return NULL;
}
@@ -1059,6 +1060,7 @@ int ip_tunnel_init_net(struct net *net, unsigned int ip_tnl_net_id,
it_init_net = net_generic(&init_net, ip_tnl_net_id);
itn->type = it_init_net->type;
itn->fb_tunnel_dev = NULL;
+ RCU_INIT_POINTER(itn->fb_tun, NULL);
return 0;
}
@@ -1074,8 +1076,9 @@ int ip_tunnel_init_net(struct net *net, unsigned int ip_tnl_net_id,
if (!IS_ERR(itn->fb_tunnel_dev)) {
itn->fb_tunnel_dev->features |= NETIF_F_NETNS_LOCAL;
itn->fb_tunnel_dev->mtu = ip_tunnel_bind_dev(itn->fb_tunnel_dev);
- ip_tunnel_add(itn, netdev_priv(itn->fb_tunnel_dev));
itn->type = itn->fb_tunnel_dev->type;
+ rcu_assign_pointer(itn->fb_tun,
+ netdev_priv(itn->fb_tunnel_dev));
}
rtnl_unlock();
@@ -1262,6 +1265,8 @@ void ip_tunnel_uninit(struct net_device *dev)
/* fb_tunnel_dev will be unregisted in net-exit call. */
if (itn->fb_tunnel_dev != dev)
ip_tunnel_del(itn, netdev_priv(dev));
+ else
+ rcu_assign_pointer(itn->fb_tun, NULL);
dst_cache_reset(&tunnel->dst_cache);
}
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
2020-06-15 15:06 [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup() Taehee Yoo
@ 2020-06-15 16:02 ` Eric Dumazet
2020-06-16 14:26 ` Taehee Yoo
0 siblings, 1 reply; 3+ messages in thread
From: Eric Dumazet @ 2020-06-15 16:02 UTC (permalink / raw)
To: Taehee Yoo, davem, kuba, netdev; +Cc: pshelar
On 6/15/20 8:06 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 is protected by RTNL. It's not enough to be used
> in the datapath.
> So, this pointer would be used after an interface is deleted.
> It eventually results in the use-after-free problem.
>
> In order to avoid the problem, the new tunnel pointer variable is added,
> which indicates a fallback tunnel device's tunnel pointer.
> This is protected by both RTNL and RCU.
> So, it's safe to be used in the datapath.
>
> 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]
> [ ... ]
>
> Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> ---
> include/net/ip_tunnels.h | 1 +
> net/ipv4/ip_tunnel.c | 11 ++++++++---
> 2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> index 076e5d7db7d3..7442c517bb75 100644
> --- a/include/net/ip_tunnels.h
> +++ b/include/net/ip_tunnels.h
> @@ -164,6 +164,7 @@ struct ip_tunnel_net {
> struct rtnl_link_ops *rtnl_link_ops;
> struct hlist_head tunnels[IP_TNL_HASH_SIZE];
> struct ip_tunnel __rcu *collect_md_tun;
> + struct ip_tunnel __rcu *fb_tun;
> int type;
> };
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index f4f1d11eab50..285b863e2fcc 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -162,8 +162,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);
> + t = rcu_dereference(itn->fb_tun);
> + if (t && t->dev->flags & IFF_UP)
> + return t;
>
There is no need for a new variable.
Your patch does not add any new rcu grace period, so it seems obvious that you
relied on existing grace periods.
The real question is why ip_tunnel_uninit() does not clear itn->fb_tunnel_dev
And of course why ip_tunnel_lookup() does not a READ_ONCE()
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index f4f1d11eab502290f9d74e2c8aafd69bceb58763..2416aa33d3645e1da967ec4c914564c5727a4d80 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -87,6 +87,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
{
unsigned int hash;
struct ip_tunnel *t, *cand = NULL;
+ struct net_device *ndev;
struct hlist_head *head;
hash = ip_tunnel_hash(key, remote);
@@ -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;
}
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup()
2020-06-15 16:02 ` Eric Dumazet
@ 2020-06-16 14:26 ` Taehee Yoo
0 siblings, 0 replies; 3+ messages in thread
From: Taehee Yoo @ 2020-06-16 14:26 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, Jakub Kicinski, Netdev, pshelar
On Tue, 16 Jun 2020 at 01:02, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
Hi Eric,
Thank you for the review!
>
> On 6/15/20 8:06 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 is protected by RTNL. It's not enough to be used
> > in the datapath.
> > So, this pointer would be used after an interface is deleted.
> > It eventually results in the use-after-free problem.
> >
> > In order to avoid the problem, the new tunnel pointer variable is added,
> > which indicates a fallback tunnel device's tunnel pointer.
> > This is protected by both RTNL and RCU.
> > So, it's safe to be used in the datapath.
> >
> > 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]
> > [ ... ]
> >
> > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > Signed-off-by: Taehee Yoo <ap420073@gmail.com>
> > ---
> > include/net/ip_tunnels.h | 1 +
> > net/ipv4/ip_tunnel.c | 11 ++++++++---
> > 2 files changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/include/net/ip_tunnels.h b/include/net/ip_tunnels.h
> > index 076e5d7db7d3..7442c517bb75 100644
> > --- a/include/net/ip_tunnels.h
> > +++ b/include/net/ip_tunnels.h
> > @@ -164,6 +164,7 @@ struct ip_tunnel_net {
> > struct rtnl_link_ops *rtnl_link_ops;
> > struct hlist_head tunnels[IP_TNL_HASH_SIZE];
> > struct ip_tunnel __rcu *collect_md_tun;
> > + struct ip_tunnel __rcu *fb_tun;
> > int type;
> > };
> >
> > diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> > index f4f1d11eab50..285b863e2fcc 100644
> > --- a/net/ipv4/ip_tunnel.c
> > +++ b/net/ipv4/ip_tunnel.c
> > @@ -162,8 +162,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);
> > + t = rcu_dereference(itn->fb_tun);
> > + if (t && t->dev->flags & IFF_UP)
> > + return t;
> >
>
> There is no need for a new variable.
>
> Your patch does not add any new rcu grace period, so it seems obvious that you
> relied on existing grace periods.
>
> The real question is why ip_tunnel_uninit() does not clear itn->fb_tunnel_dev
>
> And of course why ip_tunnel_lookup() does not a READ_ONCE()
>
> diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
> index f4f1d11eab502290f9d74e2c8aafd69bceb58763..2416aa33d3645e1da967ec4c914564c5727a4d80 100644
> --- a/net/ipv4/ip_tunnel.c
> +++ b/net/ipv4/ip_tunnel.c
> @@ -87,6 +87,7 @@ struct ip_tunnel *ip_tunnel_lookup(struct ip_tunnel_net *itn,
> {
> unsigned int hash;
> struct ip_tunnel *t, *cand = NULL;
> + struct net_device *ndev;
> struct hlist_head *head;
>
> hash = ip_tunnel_hash(key, remote);
> @@ -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;
> }
>
Thank you for the suggestion.
I tested this approach and it works well,
So, I will send a v2 patch soon.
Thanks a lot!
Taehee Yoo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-06-16 14:26 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-15 15:06 [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup() Taehee Yoo
2020-06-15 16:02 ` Eric Dumazet
2020-06-16 14:26 ` Taehee Yoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.