netdev - Re: [PATCH net v2] 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] [day] [month] [year] [list]

Message-ID: <CAMArcTXY4NNBuJPgj1YW0EH=pivom9MM96nSUsxrg=sgY_BXcQ@mail.gmail.com>
Date:   Wed, 17 Jun 2020 01:37:41 +0900
From:   Taehee Yoo <ap420073@...il.com>
To:     Eric Dumazet <eric.dumazet@...il.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Netdev <netdev@...r.kernel.org>, pshelar@...ira.com
Subject: Re: [PATCH net v2] ip_tunnel: fix use-after-free in ip_tunnel_lookup()

On Wed, 17 Jun 2020 at 01:16, Eric Dumazet <eric.dumazet@...il.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@...il.com>
> > Fixes: c54419321455 ("GRE: Refactor GRE tunneling code.")
> > Signed-off-by: Taehee Yoo <ap420073@...il.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