netdev - Re: [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Mon, 15 Jun 2020 09:02:27 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Taehee Yoo <ap420073@...il.com>, davem@...emloft.net,
        kuba@...nel.org, netdev@...r.kernel.org
Cc:     pshelar@...ira.com
Subject: Re: [PATCH net] ip_tunnel: fix use-after-free in ip_tunnel_lookup()



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@...il.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;
 }