[PATCH net] ip6_gre: fix use-after-free in ip6gre_tunnel_lookup()

[PATCH net] ip6_gre: fix use-after-free in ip6gre_tunnel_lookup()

[Taehee Yoo]

In the datapath, the ip6gre_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 ip6gre1 type ip6gre local fc:0::1 \
	    remote fc:0::2
    ip netns exec A ip -6 a a fc:100::1/64 dev ip6gre1
    ip netns exec A ip link set ip6gre1 up
    ip netns exec A ip -6 a a fc:0::1/64 dev eth0
    ip netns exec A ip link set ip6gre0 up

    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 ip6gre1 type ip6gre local fc:0::2 \
	    remote fc:0::1
    ip netns exec B ip -6 a a fc:100::2/64 dev ip6gre1
    ip netns exec B ip link set ip6gre1 up
    ip netns exec B ip -6 a a fc:0::2/64 dev eth1
    ip netns exec B ip link set ip6gre0 up
    ip netns exec A ping fc:100::2 -s 60000 &
    ip netns del B

Splat looks like:
[   73.087285][    C1] BUG: KASAN: use-after-free in ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.088361][    C1] Read of size 4 at addr ffff888040559218 by task ping/1429
[   73.089317][    C1]
[   73.089638][    C1] CPU: 1 PID: 1429 Comm: ping Not tainted 5.7.0+ #602
[   73.090531][    C1] Hardware name: innotek GmbH VirtualBox/VirtualBox, BIOS VirtualBox 12/01/2006
[   73.091725][    C1] Call Trace:
[   73.092160][    C1]  <IRQ>
[   73.092556][    C1]  dump_stack+0x96/0xdb
[   73.093122][    C1]  print_address_description.constprop.6+0x2cc/0x450
[   73.094016][    C1]  ? ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.094894][    C1]  ? ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.095767][    C1]  ? ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.096619][    C1]  kasan_report+0x154/0x190
[   73.097209][    C1]  ? ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.097989][    C1]  ip6gre_tunnel_lookup+0x1064/0x13f0 [ip6_gre]
[   73.098750][    C1]  ? gre_del_protocol+0x60/0x60 [gre]
[   73.099500][    C1]  gre_rcv+0x1c5/0x1450 [ip6_gre]
[   73.100199][    C1]  ? ip6gre_header+0xf00/0xf00 [ip6_gre]
[   73.100985][    C1]  ? rcu_read_lock_sched_held+0xc0/0xc0
[   73.101830][    C1]  ? ip6_input_finish+0x5/0xf0
[   73.102483][    C1]  ip6_protocol_deliver_rcu+0xcbb/0x1510
[   73.103296][    C1]  ip6_input_finish+0x5b/0xf0
[   73.103920][    C1]  ip6_input+0xcd/0x2c0
[   73.104473][    C1]  ? ip6_input_finish+0xf0/0xf0
[   73.105115][    C1]  ? rcu_read_lock_held+0x90/0xa0
[   73.105783][    C1]  ? rcu_read_lock_sched_held+0xc0/0xc0
[   73.106548][    C1]  ipv6_rcv+0x1f1/0x300
[ ... ]

Fixes: c12b395a4664 ("gre: Support GRE over IPv6")
Signed-off-by: Taehee Yoo <ap420073@gmail.com>
---
 net/ipv6/ip6_gre.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 781ca8c07a0d..6506ade70f3f 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -67,6 +67,7 @@ struct ip6gre_net {
 
 	struct ip6_tnl __rcu *collect_md_tun;
 	struct ip6_tnl __rcu *collect_md_tun_erspan;
+	struct ip6_tnl __rcu *fb_tun;
 	struct net_device *fb_tunnel_dev;
 };
 
@@ -238,9 +239,9 @@ static struct ip6_tnl *ip6gre_tunnel_lookup(struct net_device *dev,
 	if (t && t->dev->flags & IFF_UP)
 		return t;
 
-	dev = ign->fb_tunnel_dev;
-	if (dev && dev->flags & IFF_UP)
-		return netdev_priv(dev);
+	t = rcu_dereference(ign->fb_tun);
+	if (t && t->dev->flags & IFF_UP)
+		return t;
 
 	return NULL;
 }
@@ -411,8 +412,12 @@ static void ip6gre_tunnel_uninit(struct net_device *dev)
 	struct ip6_tnl *t = netdev_priv(dev);
 	struct ip6gre_net *ign = net_generic(t->net, ip6gre_net_id);
 
-	ip6gre_tunnel_unlink_md(ign, t);
-	ip6gre_tunnel_unlink(ign, t);
+	if (dev == ign->fb_tunnel_dev) {
+		RCU_INIT_POINTER(ign->fb_tun, NULL);
+	} else {
+		ip6gre_tunnel_unlink_md(ign, t);
+		ip6gre_tunnel_unlink(ign, t);
+	}
 	dst_cache_reset(&t->dst_cache);
 	dev_put(dev);
 }
@@ -1584,7 +1589,7 @@ static int __net_init ip6gre_init_net(struct net *net)
 	if (err)
 		goto err_reg_dev;
 
-	rcu_assign_pointer(ign->tunnels_wc[0],
+	rcu_assign_pointer(ign->fb_tun,
 			   netdev_priv(ign->fb_tunnel_dev));
 	return 0;
 
-- 
2.17.1

Re: [PATCH net] ip6_gre: fix use-after-free in ip6gre_tunnel_lookup()

[David Miller]

From: Taehee Yoo <ap420073@gmail.com>
Date: Mon, 15 Jun 2020 15:07:51 +0000

> In the datapath, the ip6gre_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.
 ...

I'm marking this changes requested because it seems like the feedback Eric
Dumazet provided for the ip_tunnel version of this fix applies here too.

Thank you.

Re: [PATCH net] ip6_gre: fix use-after-free in ip6gre_tunnel_lookup()

[Taehee Yoo]

On Tue, 16 Jun 2020 at 10:17, David Miller <davem@davemloft.net> wrote:
>

Hi David,
Thank you for the review :)

> From: Taehee Yoo <ap420073@gmail.com>
> Date: Mon, 15 Jun 2020 15:07:51 +0000
>
> > In the datapath, the ip6gre_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.
>  ...
>
> I'm marking this changes requested because it seems like the feedback Eric
> Dumazet provided for the ip_tunnel version of this fix applies here too.
>
> Thank you.

I will send a v2 patch soon.

Thanks a lot!
Taehee Yoo