netdev - Re: [PATCH net] ipv6: prevent possible fib6 leaks

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAEA6p_CWuuQk0KxZ=yf+TuNdSUWq1zD0gtejUVyzczinBot8JA@mail.gmail.com>
Date:   Wed, 15 May 2019 21:15:50 -0700
From:   Wei Wang <weiwan@...gle.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        syzbot <syzkaller@...glegroups.com>,
        David Ahern <dsahern@...il.com>, Martin Lau <kafai@...com>
Subject: Re: [PATCH net] ipv6: prevent possible fib6 leaks

On Wed, May 15, 2019 at 7:40 PM Eric Dumazet <edumazet@...gle.com> wrote:
>
> At ipv6 route dismantle, fib6_drop_pcpu_from() is responsible
> for finding all percpu routes and set their ->from pointer
> to NULL, so that fib6_ref can reach its expected value (1).
>
> The problem right now is that other cpus can still catch the
> route being deleted, since there is no rcu grace period
> between the route deletion and call to fib6_drop_pcpu_from()
>
> This can leak the fib6 and associated resources, since no
> notifier will take care of removing the last reference(s).
>
> I decided to add another boolean (fib6_destroying) instead
> of reusing/renaming exception_bucket_flushed to ease stable backports,
> and properly document the memory barriers used to implement this fix.
>
> This patch has been co-developped with Wei Wang.
>
> Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
> Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> Reported-by: syzbot <syzkaller@...glegroups.com>
> Cc: Wei Wang <weiwan@...gle.com>
> Cc: David Ahern <dsahern@...il.com>
> Cc: Martin Lau <kafai@...com>
> ---
Thanks for the fix Eric.

Acked-by: Wei Wang <weiwan@...gle.com>

>  include/net/ip6_fib.h |  3 ++-
>  net/ipv6/ip6_fib.c    | 12 +++++++++---
>  net/ipv6/route.c      |  7 +++++++
>  3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
> index 40105738e2f6b8e37adac1ff46879ce6c09381b8..525f701653ca69596b941f5f3b4a438d634c4e6c 100644
> --- a/include/net/ip6_fib.h
> +++ b/include/net/ip6_fib.h
> @@ -167,7 +167,8 @@ struct fib6_info {
>                                         dst_nocount:1,
>                                         dst_nopolicy:1,
>                                         dst_host:1,
> -                                       unused:3;
> +                                       fib6_destroying:1,
> +                                       unused:2;
>
>         struct fib6_nh                  fib6_nh;
>         struct rcu_head                 rcu;
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index 08e0390e001c270ae21013f3fd3ef3bf2a52e039..008421b550c6bfd449665aa5e7ba5505fcabe53d 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -904,6 +904,12 @@ static void fib6_drop_pcpu_from(struct fib6_info *f6i,
>  {
>         int cpu;
>
> +       /* Make sure rt6_make_pcpu_route() wont add other percpu routes
> +        * while we are cleaning them here.
> +        */
> +       f6i->fib6_destroying = 1;
> +       mb(); /* paired with the cmpxchg() in rt6_make_pcpu_route() */
> +
>         /* release the reference to this fib entry from
>          * all of its cached pcpu routes
>          */
> @@ -927,6 +933,9 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
>  {
>         struct fib6_table *table = rt->fib6_table;
>
> +       if (rt->rt6i_pcpu)
> +               fib6_drop_pcpu_from(rt, table);
> +
>         if (refcount_read(&rt->fib6_ref) != 1) {
>                 /* This route is used as dummy address holder in some split
>                  * nodes. It is not leaked, but it still holds other resources,
> @@ -948,9 +957,6 @@ static void fib6_purge_rt(struct fib6_info *rt, struct fib6_node *fn,
>                         fn = rcu_dereference_protected(fn->parent,
>                                     lockdep_is_held(&table->tb6_lock));
>                 }
> -
> -               if (rt->rt6i_pcpu)
> -                       fib6_drop_pcpu_from(rt, table);
>         }
>  }
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 23a20d62daac29e3252725b8cf95d1d1c2b567c4..27c0cc5d9d30e3689ebe6b8428cd4c586669d808 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -1295,6 +1295,13 @@ static struct rt6_info *rt6_make_pcpu_route(struct net *net,
>         prev = cmpxchg(p, NULL, pcpu_rt);
>         BUG_ON(prev);
>
> +       if (res->f6i->fib6_destroying) {
> +               struct fib6_info *from;
> +
> +               from = xchg((__force struct fib6_info **)&pcpu_rt->from, NULL);
> +               fib6_info_release(from);
> +       }
> +
>         return pcpu_rt;
>  }
>
> --
> 2.21.0.1020.gf2820cf01a-goog
>