* [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
@ 2018-06-03 13:35 Eric Dumazet
2018-06-03 14:01 ` David Ahern
0 siblings, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-06-03 13:35 UTC (permalink / raw)
To: David S . Miller; +Cc: netdev, Eric Dumazet, Eric Dumazet, David Ahern
syzbot reported a use-after-free [1]
Issue here is that rt_last might have been freed already.
We need to grab a refcount on it to prevent this.
[1]
BUG: KASAN: use-after-free in ip6_route_mpath_notify+0xe9/0x100 net/ipv6/route.c:4180
Read of size 4 at addr ffff8801bf789cf0 by task syz-executor756/4555
CPU: 1 PID: 4555 Comm: syz-executor756 Not tainted 4.17.0-rc7+ #78
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1b9/0x294 lib/dump_stack.c:113
print_address_description+0x6c/0x20b mm/kasan/report.c:256
kasan_report_error mm/kasan/report.c:354 [inline]
kasan_report.cold.7+0x242/0x2fe mm/kasan/report.c:412
__asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:432
ip6_route_mpath_notify+0xe9/0x100 net/ipv6/route.c:4180
ip6_route_multipath_add+0x615/0x1910 net/ipv6/route.c:4303
inet6_rtm_newroute+0xe3/0x160 net/ipv6/route.c:4391
rtnetlink_rcv_msg+0x466/0xc10 net/core/rtnetlink.c:4646
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2448
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4664
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x58b/0x740 net/netlink/af_netlink.c:1336
netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901
sock_sendmsg_nosec net/socket.c:629 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:639
___sys_sendmsg+0x805/0x940 net/socket.c:2117
__sys_sendmsg+0x115/0x270 net/socket.c:2155
__do_sys_sendmsg net/socket.c:2164 [inline]
__se_sys_sendmsg net/socket.c:2162 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x441819
RSP: 002b:00007ffe841e19d8 EFLAGS: 00000217 ORIG_RAX: 000000000000002e
RAX: ffffffffffffffda RBX: 0000000000000003 RCX: 0000000000441819
RDX: 0000000000000000 RSI: 0000000020000080 RDI: 0000000000000004
RBP: 00000000006cd018 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000217 R12: 0000000000402510
R13: 00000000004025a0 R14: 0000000000000000 R15: 0000000000000000
Allocated by task 4555:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
kasan_kmalloc+0xc4/0xe0 mm/kasan/kasan.c:553
kasan_slab_alloc+0x12/0x20 mm/kasan/kasan.c:490
kmem_cache_alloc+0x12e/0x760 mm/slab.c:3554
dst_alloc+0xbb/0x1d0 net/core/dst.c:104
__ip6_dst_alloc+0x35/0xa0 net/ipv6/route.c:361
ip6_dst_alloc+0x29/0xb0 net/ipv6/route.c:376
ip6_route_info_create+0x4d4/0x3a30 net/ipv6/route.c:2834
ip6_route_multipath_add+0xc7e/0x1910 net/ipv6/route.c:4240
inet6_rtm_newroute+0xe3/0x160 net/ipv6/route.c:4391
rtnetlink_rcv_msg+0x466/0xc10 net/core/rtnetlink.c:4646
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2448
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4664
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x58b/0x740 net/netlink/af_netlink.c:1336
netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901
sock_sendmsg_nosec net/socket.c:629 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:639
___sys_sendmsg+0x805/0x940 net/socket.c:2117
__sys_sendmsg+0x115/0x270 net/socket.c:2155
__do_sys_sendmsg net/socket.c:2164 [inline]
__se_sys_sendmsg net/socket.c:2162 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Freed by task 4555:
save_stack+0x43/0xd0 mm/kasan/kasan.c:448
set_track mm/kasan/kasan.c:460 [inline]
__kasan_slab_free+0x11a/0x170 mm/kasan/kasan.c:521
kasan_slab_free+0xe/0x10 mm/kasan/kasan.c:528
__cache_free mm/slab.c:3498 [inline]
kmem_cache_free+0x86/0x2d0 mm/slab.c:3756
dst_destroy+0x267/0x3c0 net/core/dst.c:140
dst_release_immediate+0x71/0x9e net/core/dst.c:205
fib6_add+0xa40/0x1650 net/ipv6/ip6_fib.c:1305
__ip6_ins_rt+0x6c/0x90 net/ipv6/route.c:1011
ip6_route_multipath_add+0x513/0x1910 net/ipv6/route.c:4267
inet6_rtm_newroute+0xe3/0x160 net/ipv6/route.c:4391
rtnetlink_rcv_msg+0x466/0xc10 net/core/rtnetlink.c:4646
netlink_rcv_skb+0x172/0x440 net/netlink/af_netlink.c:2448
rtnetlink_rcv+0x1c/0x20 net/core/rtnetlink.c:4664
netlink_unicast_kernel net/netlink/af_netlink.c:1310 [inline]
netlink_unicast+0x58b/0x740 net/netlink/af_netlink.c:1336
netlink_sendmsg+0x9f0/0xfa0 net/netlink/af_netlink.c:1901
sock_sendmsg_nosec net/socket.c:629 [inline]
sock_sendmsg+0xd5/0x120 net/socket.c:639
___sys_sendmsg+0x805/0x940 net/socket.c:2117
__sys_sendmsg+0x115/0x270 net/socket.c:2155
__do_sys_sendmsg net/socket.c:2164 [inline]
__se_sys_sendmsg net/socket.c:2162 [inline]
__x64_sys_sendmsg+0x78/0xb0 net/socket.c:2162
do_syscall_64+0x1b1/0x800 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x49/0xbe
The buggy address belongs to the object at ffff8801bf789c40
which belongs to the cache ip6_dst_cache of size 320
The buggy address is located 176 bytes inside of
320-byte region [ffff8801bf789c40, ffff8801bf789d80)
The buggy address belongs to the page:
page:ffffea0006fde240 count:1 mapcount:0 mapping:ffff8801bf789040 index:0x0
flags: 0x2fffc0000000100(slab)
raw: 02fffc0000000100 ffff8801bf789040 0000000000000000 000000010000000a
raw: ffffea0006f92f20 ffff8801cd9e7248 ffff8801cda00c40 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff8801bf789b80: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
ffff8801bf789c00: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
>ffff8801bf789c80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
^
ffff8801bf789d00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
ffff8801bf789d80: fc fc fc fc fc fc fc fc 00 00 00 00 00 00 00 00
Fixes: 3b1137fe7482 ("net: ipv6: Change notifications for multipath add to RTA_MULTIPATH")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: David Ahern <dsa@cumulusnetworks.com>
Reported-by: syzbot <syzkaller@googlegroups.com>
---
net/ipv6/route.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
err_nh = NULL;
list_for_each_entry(nh, &rt6_nh_list, next) {
+ dst_release(&rt_last->dst);
rt_last = nh->rt6_info;
+ dst_hold(&rt_last->dst);
err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
/* save reference to first route for notification */
if (!rt_notif && !err)
@@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
list_del(&nh->next);
kfree(nh);
}
-
+ dst_release(&rt_last->dst);
return err;
}
--
2.17.1.1185.g55be947832-goog
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
2018-06-03 13:35 [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify() Eric Dumazet
@ 2018-06-03 14:01 ` David Ahern
2018-06-03 14:31 ` Eric Dumazet
2018-06-03 14:40 ` David Ahern
0 siblings, 2 replies; 6+ messages in thread
From: David Ahern @ 2018-06-03 14:01 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet
On 6/3/18 7:35 AM, Eric Dumazet wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>
> err_nh = NULL;
> list_for_each_entry(nh, &rt6_nh_list, next) {
> + dst_release(&rt_last->dst);
> rt_last = nh->rt6_info;
> + dst_hold(&rt_last->dst);
> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
> /* save reference to first route for notification */
> if (!rt_notif && !err)
> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
> list_del(&nh->next);
> kfree(nh);
> }
> -
> + dst_release(&rt_last->dst);
> return err;
> }
Since the rtnl lock is held, a successfully inserted route can not be
removed until ip6_route_multipath_add finishes. This is a simpler change
that works with net-next as well:
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index f4d61736c41a..1684197c189f 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4263,11 +4263,12 @@ static int ip6_route_multipath_add(struct
fib6_config *cfg,
err_nh = NULL;
list_for_each_entry(nh, &rt6_nh_list, next) {
- rt_last = nh->rt6_info;
err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
/* save reference to first route for notification */
if (!rt_notif && !err)
rt_notif = nh->rt6_info;
+ if (!err)
+ rt_last = nh->rt6_info;
/* nh->rt6_info is used or freed at this point, reset to
NULL*/
nh->rt6_info = NULL;
Is there a reproducer for the syzbot case?
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
2018-06-03 14:01 ` David Ahern
@ 2018-06-03 14:31 ` Eric Dumazet
2018-06-03 14:46 ` David Ahern
2018-06-03 14:40 ` David Ahern
1 sibling, 1 reply; 6+ messages in thread
From: Eric Dumazet @ 2018-06-03 14:31 UTC (permalink / raw)
To: David Ahern, Eric Dumazet, David S . Miller; +Cc: netdev
On 06/03/2018 07:01 AM, David Ahern wrote:
> On 6/3/18 7:35 AM, Eric Dumazet wrote:
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>>
>> err_nh = NULL;
>> list_for_each_entry(nh, &rt6_nh_list, next) {
>> + dst_release(&rt_last->dst);
>> rt_last = nh->rt6_info;
>> + dst_hold(&rt_last->dst);
>> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
>> /* save reference to first route for notification */
>> if (!rt_notif && !err)
>> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>> list_del(&nh->next);
>> kfree(nh);
>> }
>> -
>> + dst_release(&rt_last->dst);
>> return err;
>> }
>
> Since the rtnl lock is held, a successfully inserted route can not be
> removed until ip6_route_multipath_add finishes. This is a simpler change
> that works with net-next as well:
Your patch changes the intent of your original commit.
It seems you wanted rt_last to point to the last attempted insertion,
not the last successful one ?
Or have I misunderstood, and not only we had a use-after-free, but also
a semantic error ?
>
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index f4d61736c41a..1684197c189f 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4263,11 +4263,12 @@ static int ip6_route_multipath_add(struct
> fib6_config *cfg,
>
> err_nh = NULL;
> list_for_each_entry(nh, &rt6_nh_list, next) {
> - rt_last = nh->rt6_info;
> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
> /* save reference to first route for notification */
> if (!rt_notif && !err)
> rt_notif = nh->rt6_info;
> + if (!err)
> + rt_last = nh->rt6_info;
>
> /* nh->rt6_info is used or freed at this point, reset to
> NULL*/
> nh->rt6_info = NULL;
>
>
> Is there a reproducer for the syzbot case?
Not yet.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
2018-06-03 14:01 ` David Ahern
2018-06-03 14:31 ` Eric Dumazet
@ 2018-06-03 14:40 ` David Ahern
1 sibling, 0 replies; 6+ messages in thread
From: David Ahern @ 2018-06-03 14:40 UTC (permalink / raw)
To: Eric Dumazet, David S . Miller; +Cc: netdev, Eric Dumazet
On 6/3/18 8:01 AM, David Ahern wrote:
> Is there a reproducer for the syzbot case?
One reproducer is to insert a route and then add a multipath route that
has a duplicate nexthop.e.g,:
ip -6 ro add vrf red 2001:db8:101::/64 nexthop via 2001:db8:1::2
ip -6 ro append vrf red 2001:db8:101::/64 nexthop via 2001:db8:1::4
nexthop via 2001:db8:1::2
Current net and next-next generates the trace; with the fix I proposed I
don't see it on either branch and I do see the expected notifications to
userspace.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
2018-06-03 14:31 ` Eric Dumazet
@ 2018-06-03 14:46 ` David Ahern
2018-06-04 2:39 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: David Ahern @ 2018-06-03 14:46 UTC (permalink / raw)
To: Eric Dumazet, Eric Dumazet, David S . Miller; +Cc: netdev
On 6/3/18 8:31 AM, Eric Dumazet wrote:
>
>
> On 06/03/2018 07:01 AM, David Ahern wrote:
>> On 6/3/18 7:35 AM, Eric Dumazet wrote:
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index f4d61736c41abe8cd7f439c4a37100e90c1eacca..830eefdbdb6734eb81ea0322fb6077ee20be1889 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -4263,7 +4263,9 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>>>
>>> err_nh = NULL;
>>> list_for_each_entry(nh, &rt6_nh_list, next) {
>>> + dst_release(&rt_last->dst);
>>> rt_last = nh->rt6_info;
>>> + dst_hold(&rt_last->dst);
>>> err = __ip6_ins_rt(nh->rt6_info, info, &nh->mxc, extack);
>>> /* save reference to first route for notification */
>>> if (!rt_notif && !err)
>>> @@ -4317,7 +4319,7 @@ static int ip6_route_multipath_add(struct fib6_config *cfg,
>>> list_del(&nh->next);
>>> kfree(nh);
>>> }
>>> -
>>> + dst_release(&rt_last->dst);
>>> return err;
>>> }
>>
>> Since the rtnl lock is held, a successfully inserted route can not be
>> removed until ip6_route_multipath_add finishes. This is a simpler change
>> that works with net-next as well:
>
> Your patch changes the intent of your original commit.
>
> It seems you wanted rt_last to point to the last attempted insertion,
> not the last successful one ?
The note in ip6_route_mpath_notify explains it:
/* if this is an APPEND route, then rt points to the first route
* inserted and rt_last points to last route inserted. Userspace
>
> Or have I misunderstood, and not only we had a use-after-free, but also
> a semantic error ?
It was a mistake to set rt_last before checking err. So the
use-after-free exposed the semantic error.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify()
2018-06-03 14:46 ` David Ahern
@ 2018-06-04 2:39 ` Eric Dumazet
0 siblings, 0 replies; 6+ messages in thread
From: Eric Dumazet @ 2018-06-04 2:39 UTC (permalink / raw)
To: David Ahern, Eric Dumazet, David S . Miller; +Cc: netdev
On 06/03/2018 07:46 AM, David Ahern wrote:
> It was a mistake to set rt_last before checking err. So the
> use-after-free exposed the semantic error.
>
SGTM, please send the formal patch then, thanks !
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-04 2:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-06-03 13:35 [PATCH net] net: ipv6: prevent use after free in ip6_route_mpath_notify() Eric Dumazet
2018-06-03 14:01 ` David Ahern
2018-06-03 14:31 ` Eric Dumazet
2018-06-03 14:46 ` David Ahern
2018-06-04 2:39 ` Eric Dumazet
2018-06-03 14:40 ` David Ahern
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.