Message-ID: <CAEA6p_BiWek-4Bu9fOs1wM+Ohc3uKc7ifAJL60UONMTgV8u6VQ@mail.gmail.com>
Date:   Fri, 19 Jan 2018 13:46:02 -0800
From:   Wei Wang <weiwan@...gle.com>
To:     Ido Schimmel <idosch@...sch.org>
Cc:     Martin KaFai Lau <kafai@...com>,
        David Miller <davem@...emloft.net>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>,
        Eric Dumazet <edumazet@...gle.com>
Subject: Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node

On Fri, Jan 19, 2018 at 1:36 PM, Wei Wang <weiwan@...gle.com> wrote:
>
>
> On Fri, Jan 19, 2018 at 1:13 PM, Ido Schimmel <idosch@...sch.org> wrote:
>> Hi Wei, Martin,
>>
>> On Thu, Jan 18, 2018 at 03:31:29PM -0800, Wei Wang wrote:
>>> On Thu, Jan 18, 2018 at 2:47 PM, Martin KaFai Lau <kafai@...com> wrote:
>>> > On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
>>> >> From: Wei Wang <weiwan@...gle.com>
>>> >>
>>> >> After commit 4512c43eac7e, if we add a route to the subtree of
>>> >> tb6_root
>>> >> which does not have any route attached to it yet, the current code
>>> >> will
>>> >> let tb6_root and the node in the subtree share the same route.
>>> >> This could cause problem cause tb6_root has RTN_INFO flag marked and
>>> >> the
>>> > You meant the RTN_RTINFO check in fib6_purge_rt()?
>>> >
>>> Yes. Exactly.
>>
>> The check in fib6_purge_rt() is indeed problematic as tb6_root will not
>> release its reference on the deleted route. I can easily reproduce that
>> on my system. However, I don't understand how come we end up with a
>> use-after-free given tb6_root takes a reference on the route?
>>

(Resending with plain txt format)

Hi Ido,

I think the use-after-free does not really happen on the route that is being
falsely shared, but on the route which that route's rt6i_next is pointing to.
Nothing could prevent rt->rt6i_next from being released.

Thanks.
Wei

>> Thanks
>>
>>>
>>> >> tree repair and clean up code will not work properly.
>>> >> This commit makes sure tb6_root->leaf points back to null_entry
>>> >> instead
>>> >> of sharing route with other node.
>>> >>
>>> >> It fixes the following syzkaller reported issue:
>>> >> BUG: KASAN: use-after-free in ipv6_prefix_equal include/net/ipv6.h:540
>>> >> [inline]
>>> >> BUG: KASAN: use-after-free in fib6_add_1+0x165f/0x1790
>>> >> net/ipv6/ip6_fib.c:618
>>> >> Read of size 8 at addr ffff8801bc043498 by task syz-executor5/19819
>>> >>
>>> >> CPU: 1 PID: 19819 Comm: syz-executor5 Not tainted 4.15.0-rc7+ #186
>>> >> Hardware name: Google Google Compute Engine/Google Compute Engine,
>>> >> BIOS Google 01/01/2011
>>> >> Call Trace:
>>> >>  __dump_stack lib/dump_stack.c:17 [inline]
>>> >>  dump_stack+0x194/0x257 lib/dump_stack.c:53
>>> >>  print_address_description+0x73/0x250 mm/kasan/report.c:252
>>> >>  kasan_report_error mm/kasan/report.c:351 [inline]
>>> >>  kasan_report+0x25b/0x340 mm/kasan/report.c:409
>>> >>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:430
>>> >>  ipv6_prefix_equal include/net/ipv6.h:540 [inline]
>>> >>  fib6_add_1+0x165f/0x1790 net/ipv6/ip6_fib.c:618
>>> >>  fib6_add+0x5fa/0x1540 net/ipv6/ip6_fib.c:1214
>>> >>  __ip6_ins_rt+0x6c/0x90 net/ipv6/route.c:1003
>>> >>  ip6_route_add+0x141/0x190 net/ipv6/route.c:2790
>>> >>  ipv6_route_ioctl+0x4db/0x6b0 net/ipv6/route.c:3299
>>> >>  inet6_ioctl+0xef/0x1e0 net/ipv6/af_inet6.c:520
>>> >>  sock_do_ioctl+0x65/0xb0 net/socket.c:958
>>> >>  sock_ioctl+0x2c2/0x440 net/socket.c:1055
>>> >>  vfs_ioctl fs/ioctl.c:46 [inline]
>>> >>  do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
>>> >>  SYSC_ioctl fs/ioctl.c:701 [inline]
>>> >>  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
>>> >>  entry_SYSCALL_64_fastpath+0x23/0x9a
>>> >> RIP: 0033:0x452ac9
>>> >> RSP: 002b:00007fd42b321c58 EFLAGS: 00000212 ORIG_RAX: 0000000000000010
>>> >> RAX: ffffffffffffffda RBX: 000000000071bea0 RCX: 0000000000452ac9
>>> >> RDX: 0000000020fd7000 RSI: 000000000000890b RDI: 0000000000000013
>>> >> RBP: 000000000000049e R08: 0000000000000000 R09: 0000000000000000
>>> >> R10: 0000000000000000 R11: 0000000000000212 R12: 00000000006f4f70
>>> >> R13: 00000000ffffffff R14: 00007fd42b3226d4 R15: 0000000000000000
>>> >>
>>> >> Fixes: 4512c43eac7e ("ipv6: remove null_entry before adding default
>>> >> route")
>>> >> Signed-off-by: Wei Wang <weiwan@...gle.com>
>>> >> Acked-by: Eric Dumazet <edumazet@...gle.com>
>>> >> ---
>>> >>  net/ipv6/ip6_fib.c | 10 ++++++++--
>>> >>  1 file changed, 8 insertions(+), 2 deletions(-)
>>> >>
>>> >> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
>>> >> index 9dcc3924a975..217683d40f12 100644
>>> >> --- a/net/ipv6/ip6_fib.c
>>> >> +++ b/net/ipv6/ip6_fib.c
>>> >> @@ -1226,8 +1226,14 @@ int fib6_add(struct fib6_node *root, struct
>>> >> rt6_info *rt,
>>> >>               }
>>> >>
>>> >>               if (!rcu_access_pointer(fn->leaf)) {
>>> >> -                     atomic_inc(&rt->rt6i_ref);
>>> >> -                     rcu_assign_pointer(fn->leaf, rt);
>>> >> +                     if (fn->fn_flags & RTN_TL_ROOT) {
>>> >> +                             /* put back null_entry for root node */
>>> >> +                             rcu_assign_pointer(fn->leaf,
>>> >> +
>>> >> info->nl_net->ipv6.ip6_null_entry);
>>> >> +                     } else {
>>> >> +                             atomic_inc(&rt->rt6i_ref);
>>> >> +                             rcu_assign_pointer(fn->leaf, rt);
>>> >> +                     }
>>> >>               }
>>> >>               fn = sn;
>>> >>       }
>>> >> --
>>> >> 2.16.0.rc1.238.g530d649a79-goog
>>> >>
>

Powered by blists - more mailing lists