* [PATCH net] ipv6: don't let tb6_root node share routes with other node
@ 2018-01-18 18:40 Wei Wang
2018-01-18 22:47 ` Martin KaFai Lau
2018-01-19 2:14 ` David Miller
0 siblings, 2 replies; 8+ messages in thread
From: Wei Wang @ 2018-01-18 18:40 UTC (permalink / raw)
To: David Miller, netdev; +Cc: Eric Dumazet, Martin KaFai Lau, Wei Wang
From: Wei Wang <weiwan@google.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
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@google.com>
Acked-by: Eric Dumazet <edumazet@google.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
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-18 18:40 [PATCH net] ipv6: don't let tb6_root node share routes with other node Wei Wang
@ 2018-01-18 22:47 ` Martin KaFai Lau
2018-01-18 23:31 ` Wei Wang
2018-01-19 2:14 ` David Miller
1 sibling, 1 reply; 8+ messages in thread
From: Martin KaFai Lau @ 2018-01-18 22:47 UTC (permalink / raw)
To: Wei Wang; +Cc: David Miller, netdev, Eric Dumazet
On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
> From: Wei Wang <weiwan@google.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()?
> 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@google.com>
> Acked-by: Eric Dumazet <edumazet@google.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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-18 22:47 ` Martin KaFai Lau
@ 2018-01-18 23:31 ` Wei Wang
2018-01-18 23:43 ` Martin KaFai Lau
2018-01-19 21:13 ` Ido Schimmel
0 siblings, 2 replies; 8+ messages in thread
From: Wei Wang @ 2018-01-18 23:31 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: David Miller, Linux Kernel Network Developers, Eric Dumazet
On Thu, Jan 18, 2018 at 2:47 PM, Martin KaFai Lau <kafai@fb.com> wrote:
> On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
>> From: Wei Wang <weiwan@google.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.
>> 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@google.com>
>> Acked-by: Eric Dumazet <edumazet@google.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
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-18 23:31 ` Wei Wang
@ 2018-01-18 23:43 ` Martin KaFai Lau
2018-01-19 21:13 ` Ido Schimmel
1 sibling, 0 replies; 8+ messages in thread
From: Martin KaFai Lau @ 2018-01-18 23:43 UTC (permalink / raw)
To: Wei Wang; +Cc: David Miller, Linux Kernel Network Developers, Eric Dumazet
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@fb.com> wrote:
> > On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
> >> From: Wei Wang <weiwan@google.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.
Looks good to me. Thanks for the fix!
Acked-by: Martin KaFai Lau <kafai@fb.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-18 18:40 [PATCH net] ipv6: don't let tb6_root node share routes with other node Wei Wang
2018-01-18 22:47 ` Martin KaFai Lau
@ 2018-01-19 2:14 ` David Miller
1 sibling, 0 replies; 8+ messages in thread
From: David Miller @ 2018-01-19 2:14 UTC (permalink / raw)
To: weiwan; +Cc: netdev, edumazet, kafai
From: Wei Wang <weiwan@google.com>
Date: Thu, 18 Jan 2018 10:40:03 -0800
> From: Wei Wang <weiwan@google.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
> 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:
...
> Fixes: 4512c43eac7e ("ipv6: remove null_entry before adding default route")
> Signed-off-by: Wei Wang <weiwan@google.com>
> Acked-by: Eric Dumazet <edumazet@google.com>
Applied, thank you.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-18 23:31 ` Wei Wang
2018-01-18 23:43 ` Martin KaFai Lau
@ 2018-01-19 21:13 ` Ido Schimmel
[not found] ` <CAEA6p_C4ctnTJWSQtnCPhRm48AhaqVLV4eQhfdP+Owv_SVVRLw@mail.gmail.com>
1 sibling, 1 reply; 8+ messages in thread
From: Ido Schimmel @ 2018-01-19 21:13 UTC (permalink / raw)
To: Wei Wang
Cc: Martin KaFai Lau, David Miller, Linux Kernel Network Developers,
Eric Dumazet
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@fb.com> wrote:
> > On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
> >> From: Wei Wang <weiwan@google.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?
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@google.com>
> >> Acked-by: Eric Dumazet <edumazet@google.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
> >>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
[not found] ` <CAEA6p_C4ctnTJWSQtnCPhRm48AhaqVLV4eQhfdP+Owv_SVVRLw@mail.gmail.com>
@ 2018-01-19 21:46 ` Wei Wang
2018-01-19 22:17 ` Ido Schimmel
0 siblings, 1 reply; 8+ messages in thread
From: Wei Wang @ 2018-01-19 21:46 UTC (permalink / raw)
To: Ido Schimmel
Cc: Martin KaFai Lau, David Miller, Linux Kernel Network Developers,
Eric Dumazet
On Fri, Jan 19, 2018 at 1:36 PM, Wei Wang <weiwan@google.com> wrote:
>
>
> On Fri, Jan 19, 2018 at 1:13 PM, Ido Schimmel <idosch@idosch.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@fb.com> wrote:
>>> > On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
>>> >> From: Wei Wang <weiwan@google.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@google.com>
>>> >> Acked-by: Eric Dumazet <edumazet@google.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
>>> >>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH net] ipv6: don't let tb6_root node share routes with other node
2018-01-19 21:46 ` Wei Wang
@ 2018-01-19 22:17 ` Ido Schimmel
0 siblings, 0 replies; 8+ messages in thread
From: Ido Schimmel @ 2018-01-19 22:17 UTC (permalink / raw)
To: Wei Wang
Cc: Martin KaFai Lau, David Miller, Linux Kernel Network Developers,
Eric Dumazet
On Fri, Jan 19, 2018 at 01:46:02PM -0800, Wei Wang wrote:
> On Fri, Jan 19, 2018 at 1:36 PM, Wei Wang <weiwan@google.com> wrote:
> >
> >
> > On Fri, Jan 19, 2018 at 1:13 PM, Ido Schimmel <idosch@idosch.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@fb.com> wrote:
> >>> > On Thu, Jan 18, 2018 at 10:40:03AM -0800, Wei Wang wrote:
> >>> >> From: Wei Wang <weiwan@google.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.
Yep, I considered it, then confused myself and disqualified the
possibility, but you're right. FWIW, here's the reproducer:
ip -6 route add default from 2001:db8::/64 dev dummy0 metric 1
ip -6 route append default from 2001:db8::/64 dev dummy0 metric 2
ip -6 route del default from 2001:db8::/64 dev dummy0 metric 1
ip -6 route del default from 2001:db8::/64 dev dummy0 metric 2
ip -6 route show
Thanks!
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-01-19 22:17 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-01-18 18:40 [PATCH net] ipv6: don't let tb6_root node share routes with other node Wei Wang
2018-01-18 22:47 ` Martin KaFai Lau
2018-01-18 23:31 ` Wei Wang
2018-01-18 23:43 ` Martin KaFai Lau
2018-01-19 21:13 ` Ido Schimmel
[not found] ` <CAEA6p_C4ctnTJWSQtnCPhRm48AhaqVLV4eQhfdP+Owv_SVVRLw@mail.gmail.com>
2018-01-19 21:46 ` Wei Wang
2018-01-19 22:17 ` Ido Schimmel
2018-01-19 2:14 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).