Re: [PATCH net] ipv6: use fib6_info_hold_safe() when necessary

2018-07-23 Thread Martin KaFai Lau
On Sat, Jul 21, 2018 at 08:56:32PM -0700, Wei Wang wrote:
> From: Wei Wang 
> 
> In the code path where only rcu read lock is held, e.g. in the route
> lookup code path, it is not safe to directly call fib6_info_hold()
> because the fib6_info may already have been deleted but still exists
> in the rcu grace period. Holding reference to it could cause double
> free and crash the kernel.
> 
> This patch adds a new function fib6_info_hold_safe() and replace
> fib6_info_hold() in all necessary places.
Acked-by: Martin KaFai Lau 


Re: [PATCH net] ipv6: use fib6_info_hold_safe() when necessary

2018-07-23 Thread David Miller
From: Wei Wang 
Date: Sat, 21 Jul 2018 20:56:32 -0700

> From: Wei Wang 
> 
> In the code path where only rcu read lock is held, e.g. in the route
> lookup code path, it is not safe to directly call fib6_info_hold()
> because the fib6_info may already have been deleted but still exists
> in the rcu grace period. Holding reference to it could cause double
> free and crash the kernel.
> 
> This patch adds a new function fib6_info_hold_safe() and replace
> fib6_info_hold() in all necessary places.
> 
> Syzbot reported 3 crash traces because of this. One of them is:
 ...
> Fixes: 93531c674315 (net/ipv6: separate handling of FIB entries from dst 
> based routes)
> Reported-by: syzbot+902e2a1bcd4f7808c...@syzkaller.appspotmail.com
> Reported-by: syzbot+8ae62d67f647abeec...@syzkaller.appspotmail.com
> Reported-by: syzbot+3f08feb1408693067...@syzkaller.appspotmail.com
> Signed-off-by: Wei Wang 
> Acked-by: Eric Dumazet 

Applied, thank you.


Re: [PATCH net] ipv6: use fib6_info_hold_safe() when necessary

2018-07-23 Thread David Ahern
On 7/21/18 9:56 PM, Wei Wang wrote:
> From: Wei Wang 
> 
> In the code path where only rcu read lock is held, e.g. in the route
> lookup code path, it is not safe to directly call fib6_info_hold()
> because the fib6_info may already have been deleted but still exists
> in the rcu grace period. Holding reference to it could cause double
> free and crash the kernel.
> 
> This patch adds a new function fib6_info_hold_safe() and replace
> fib6_info_hold() in all necessary places.
> 
> Syzbot reported 3 crash traces because of this. One of them is:
> 8021q: adding VLAN 0 to HW filter on device team0
> IPv6: ADDRCONF(NETDEV_CHANGE): team0: link becomes ready
> dst_release: dst:(ptrval) refcnt:-1
> dst_release: dst:(ptrval) refcnt:-2
> WARNING: CPU: 1 PID: 4845 at include/net/dst.h:239 dst_hold 
> include/net/dst.h:239 [inline]
> WARNING: CPU: 1 PID: 4845 at include/net/dst.h:239 
> ip6_setup_cork+0xd66/0x1830 net/ipv6/ip6_output.c:1204
> dst_release: dst:(ptrval) refcnt:-1
> Kernel panic - not syncing: panic_on_warn set ...
> 

...

> 
> Fixes: 93531c674315 (net/ipv6: separate handling of FIB entries from dst 
> based routes)
> Reported-by: syzbot+902e2a1bcd4f7808c...@syzkaller.appspotmail.com
> Reported-by: syzbot+8ae62d67f647abeec...@syzkaller.appspotmail.com
> Reported-by: syzbot+3f08feb1408693067...@syzkaller.appspotmail.com
> Signed-off-by: Wei Wang 
> Acked-by: Eric Dumazet 
> ---
>  include/net/ip6_fib.h |  5 +
>  net/ipv6/addrconf.c   |  3 ++-
>  net/ipv6/route.c  | 41 +++--
>  3 files changed, 38 insertions(+), 11 deletions(-)

Reviewed-by: David Ahern 

Thanks for fixing.


[PATCH net] ipv6: use fib6_info_hold_safe() when necessary

2018-07-21 Thread Wei Wang
From: Wei Wang 

In the code path where only rcu read lock is held, e.g. in the route
lookup code path, it is not safe to directly call fib6_info_hold()
because the fib6_info may already have been deleted but still exists
in the rcu grace period. Holding reference to it could cause double
free and crash the kernel.

This patch adds a new function fib6_info_hold_safe() and replace
fib6_info_hold() in all necessary places.

Syzbot reported 3 crash traces because of this. One of them is:
8021q: adding VLAN 0 to HW filter on device team0
IPv6: ADDRCONF(NETDEV_CHANGE): team0: link becomes ready
dst_release: dst:(ptrval) refcnt:-1
dst_release: dst:(ptrval) refcnt:-2
WARNING: CPU: 1 PID: 4845 at include/net/dst.h:239 dst_hold 
include/net/dst.h:239 [inline]
WARNING: CPU: 1 PID: 4845 at include/net/dst.h:239 ip6_setup_cork+0xd66/0x1830 
net/ipv6/ip6_output.c:1204
dst_release: dst:(ptrval) refcnt:-1
Kernel panic - not syncing: panic_on_warn set ...

CPU: 1 PID: 4845 Comm: syz-executor493 Not tainted 4.18.0-rc3+ #10
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+0x1c9/0x2b4 lib/dump_stack.c:113
 panic+0x238/0x4e7 kernel/panic.c:184
dst_release: dst:(ptrval) refcnt:-2
dst_release: dst:(ptrval) refcnt:-3
 __warn.cold.8+0x163/0x1ba kernel/panic.c:536
dst_release: dst:(ptrval) refcnt:-4
 report_bug+0x252/0x2d0 lib/bug.c:186
 fixup_bug arch/x86/kernel/traps.c:178 [inline]
 do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
dst_release: dst:(ptrval) refcnt:-5
 do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
 invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:992
RIP: 0010:dst_hold include/net/dst.h:239 [inline]
RIP: 0010:ip6_setup_cork+0xd66/0x1830 net/ipv6/ip6_output.c:1204
Code: c1 ed 03 89 9d 18 ff ff ff 48 b8 00 00 00 00 00 fc ff df 41 c6 44 05 00 
f8 e9 2d 01 00 00 4c 8b a5 c8 fe ff ff e8 1a f6 e6 fa <0f> 0b e9 6a fc ff ff e8 
0e f6 e6 fa 48 8b 85 d0 fe ff ff 48 8d 78
RSP: 0018:8801a8fcf178 EFLAGS: 00010293
RAX: 8801a8eba5c0 RBX:  RCX: 869511e6
RDX:  RSI: 869515b6 RDI: 0005
RBP: 8801a8fcf2c8 R08: 8801a8eba5c0 R09: ed0035ac8338
R10: ed0035ac8338 R11: 8801ad6419c3 R12: 8801a8fcf720
R13: 8801a8fcf6a0 R14: 8801ad6419c0 R15: 8801ad641980
 ip6_make_skb+0x2c8/0x600 net/ipv6/ip6_output.c:1768
 udpv6_sendmsg+0x2c90/0x35f0 net/ipv6/udp.c:1376
 inet_sendmsg+0x1a1/0x690 net/ipv4/af_inet.c:798
 sock_sendmsg_nosec net/socket.c:641 [inline]
 sock_sendmsg+0xd5/0x120 net/socket.c:651
 ___sys_sendmsg+0x51d/0x930 net/socket.c:2125
 __sys_sendmmsg+0x240/0x6f0 net/socket.c:2220
 __do_sys_sendmmsg net/socket.c:2249 [inline]
 __se_sys_sendmmsg net/socket.c:2246 [inline]
 __x64_sys_sendmmsg+0x9d/0x100 net/socket.c:2246
 do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
 entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x446ba9
Code: e8 cc bb 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 
89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 
eb 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:7fb39a469da8 EFLAGS: 0246 ORIG_RAX: 0133
RAX: ffda RBX: 006dcc54 RCX: 00446ba9
RDX: 00b8 RSI: 20001b00 RDI: 0003
RBP: 006dcc50 R08: 7fb39a46a700 R09: 
R10:  R11: 0246 R12: 45c828efc7a64843
R13: e6eeb815b9d8a477 R14: 5068caf6f713c6fc R15: 0001
Dumping ftrace buffer:
   (ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..

Fixes: 93531c674315 (net/ipv6: separate handling of FIB entries from dst based 
routes)
Reported-by: syzbot+902e2a1bcd4f7808c...@syzkaller.appspotmail.com
Reported-by: syzbot+8ae62d67f647abeec...@syzkaller.appspotmail.com
Reported-by: syzbot+3f08feb1408693067...@syzkaller.appspotmail.com
Signed-off-by: Wei Wang 
Acked-by: Eric Dumazet 
---
 include/net/ip6_fib.h |  5 +
 net/ipv6/addrconf.c   |  3 ++-
 net/ipv6/route.c  | 41 +++--
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/include/net/ip6_fib.h b/include/net/ip6_fib.h
index 71b9043aa0e7..3d4930528db0 100644
--- a/include/net/ip6_fib.h
+++ b/include/net/ip6_fib.h
@@ -281,6 +281,11 @@ static inline void fib6_info_hold(struct fib6_info *f6i)
atomic_inc(>fib6_ref);
 }
 
+static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
+{
+   return atomic_inc_not_zero(>fib6_ref);
+}
+
 static inline void fib6_info_release(struct fib6_info *f6i)
 {
if (f6i && atomic_dec_and_test(>fib6_ref))
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 91580c62bb86..f66a1cae3366 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -2374,7 +2374,8 @@ static struct fib6_info *addrconf_get_prefix_route(const 
struct