Commit e873e4b9 authored by Wei Wang's avatar Wei Wang Committed by David S. Miller

ipv6: use fib6_info_hold_safe() when necessary

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:ffff8801a8fcf178 EFLAGS: 00010293
RAX: ffff8801a8eba5c0 RBX: 0000000000000000 RCX: ffffffff869511e6
RDX: 0000000000000000 RSI: ffffffff869515b6 RDI: 0000000000000005
RBP: ffff8801a8fcf2c8 R08: ffff8801a8eba5c0 R09: ffffed0035ac8338
R10: ffffed0035ac8338 R11: ffff8801ad6419c3 R12: ffff8801a8fcf720
R13: ffff8801a8fcf6a0 R14: ffff8801ad6419c0 R15: ffff8801ad641980
 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:00007fb39a469da8 EFLAGS: 00000246 ORIG_RAX: 0000000000000133
RAX: ffffffffffffffda RBX: 00000000006dcc54 RCX: 0000000000446ba9
RDX: 00000000000000b8 RSI: 0000000020001b00 RDI: 0000000000000003
RBP: 00000000006dcc50 R08: 00007fb39a46a700 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 45c828efc7a64843
R13: e6eeb815b9d8a477 R14: 5068caf6f713c6fc R15: 0000000000000001
Dumping ftrace buffer:
   (ftrace buffer empty)
Kernel Offset: disabled
Rebooting in 86400 seconds..

Fixes: 93531c67 ("net/ipv6: separate handling of FIB entries from dst based routes")
Reported-by: syzbot+902e2a1bcd4f7808cef5@syzkaller.appspotmail.com
Reported-by: syzbot+8ae62d67f647abeeceb9@syzkaller.appspotmail.com
Reported-by: syzbot+3f08feb14086930677d0@syzkaller.appspotmail.com
Signed-off-by: default avatarWei Wang <weiwan@google.com>
Acked-by: default avatarEric Dumazet <edumazet@google.com>
Reviewed-by: default avatarDavid Ahern <dsahern@gmail.com>
Signed-off-by: default avatarDavid S. Miller <davem@davemloft.net>
parent 5302a84e
...@@ -281,6 +281,11 @@ static inline void fib6_info_hold(struct fib6_info *f6i) ...@@ -281,6 +281,11 @@ static inline void fib6_info_hold(struct fib6_info *f6i)
atomic_inc(&f6i->fib6_ref); atomic_inc(&f6i->fib6_ref);
} }
static inline bool fib6_info_hold_safe(struct fib6_info *f6i)
{
return atomic_inc_not_zero(&f6i->fib6_ref);
}
static inline void fib6_info_release(struct fib6_info *f6i) static inline void fib6_info_release(struct fib6_info *f6i)
{ {
if (f6i && atomic_dec_and_test(&f6i->fib6_ref)) if (f6i && atomic_dec_and_test(&f6i->fib6_ref))
......
...@@ -2374,7 +2374,8 @@ static struct fib6_info *addrconf_get_prefix_route(const struct in6_addr *pfx, ...@@ -2374,7 +2374,8 @@ static struct fib6_info *addrconf_get_prefix_route(const struct in6_addr *pfx,
continue; continue;
if ((rt->fib6_flags & noflags) != 0) if ((rt->fib6_flags & noflags) != 0)
continue; continue;
fib6_info_hold(rt); if (!fib6_info_hold_safe(rt))
continue;
break; break;
} }
out: out:
......
...@@ -972,10 +972,10 @@ static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort) ...@@ -972,10 +972,10 @@ static void ip6_rt_init_dst(struct rt6_info *rt, struct fib6_info *ort)
rt->dst.lastuse = jiffies; rt->dst.lastuse = jiffies;
} }
/* Caller must already hold reference to @from */
static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from) static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
{ {
rt->rt6i_flags &= ~RTF_EXPIRES; rt->rt6i_flags &= ~RTF_EXPIRES;
fib6_info_hold(from);
rcu_assign_pointer(rt->from, from); rcu_assign_pointer(rt->from, from);
dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true); dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true);
if (from->fib6_metrics != &dst_default_metrics) { if (from->fib6_metrics != &dst_default_metrics) {
...@@ -984,6 +984,7 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from) ...@@ -984,6 +984,7 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
} }
} }
/* Caller must already hold reference to @ort */
static void ip6_rt_copy_init(struct rt6_info *rt, struct fib6_info *ort) static void ip6_rt_copy_init(struct rt6_info *rt, struct fib6_info *ort)
{ {
struct net_device *dev = fib6_info_nh_dev(ort); struct net_device *dev = fib6_info_nh_dev(ort);
...@@ -1044,9 +1045,14 @@ static struct rt6_info *ip6_create_rt_rcu(struct fib6_info *rt) ...@@ -1044,9 +1045,14 @@ static struct rt6_info *ip6_create_rt_rcu(struct fib6_info *rt)
struct net_device *dev = rt->fib6_nh.nh_dev; struct net_device *dev = rt->fib6_nh.nh_dev;
struct rt6_info *nrt; struct rt6_info *nrt;
if (!fib6_info_hold_safe(rt))
return NULL;
nrt = ip6_dst_alloc(dev_net(dev), dev, flags); nrt = ip6_dst_alloc(dev_net(dev), dev, flags);
if (nrt) if (nrt)
ip6_rt_copy_init(nrt, rt); ip6_rt_copy_init(nrt, rt);
else
fib6_info_release(rt);
return nrt; return nrt;
} }
...@@ -1178,10 +1184,15 @@ static struct rt6_info *ip6_rt_cache_alloc(struct fib6_info *ort, ...@@ -1178,10 +1184,15 @@ static struct rt6_info *ip6_rt_cache_alloc(struct fib6_info *ort,
* Clone the route. * Clone the route.
*/ */
if (!fib6_info_hold_safe(ort))
return NULL;
dev = ip6_rt_get_dev_rcu(ort); dev = ip6_rt_get_dev_rcu(ort);
rt = ip6_dst_alloc(dev_net(dev), dev, 0); rt = ip6_dst_alloc(dev_net(dev), dev, 0);
if (!rt) if (!rt) {
fib6_info_release(ort);
return NULL; return NULL;
}
ip6_rt_copy_init(rt, ort); ip6_rt_copy_init(rt, ort);
rt->rt6i_flags |= RTF_CACHE; rt->rt6i_flags |= RTF_CACHE;
...@@ -1210,12 +1221,17 @@ static struct rt6_info *ip6_rt_pcpu_alloc(struct fib6_info *rt) ...@@ -1210,12 +1221,17 @@ static struct rt6_info *ip6_rt_pcpu_alloc(struct fib6_info *rt)
struct net_device *dev; struct net_device *dev;
struct rt6_info *pcpu_rt; struct rt6_info *pcpu_rt;
if (!fib6_info_hold_safe(rt))
return NULL;
rcu_read_lock(); rcu_read_lock();
dev = ip6_rt_get_dev_rcu(rt); dev = ip6_rt_get_dev_rcu(rt);
pcpu_rt = ip6_dst_alloc(dev_net(dev), dev, flags); pcpu_rt = ip6_dst_alloc(dev_net(dev), dev, flags);
rcu_read_unlock(); rcu_read_unlock();
if (!pcpu_rt) if (!pcpu_rt) {
fib6_info_release(rt);
return NULL; return NULL;
}
ip6_rt_copy_init(pcpu_rt, rt); ip6_rt_copy_init(pcpu_rt, rt);
pcpu_rt->rt6i_flags |= RTF_PCPU; pcpu_rt->rt6i_flags |= RTF_PCPU;
return pcpu_rt; return pcpu_rt;
...@@ -2486,7 +2502,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net, ...@@ -2486,7 +2502,7 @@ static struct rt6_info *__ip6_route_redirect(struct net *net,
out: out:
if (ret) if (ret)
dst_hold(&ret->dst); ip6_hold_safe(net, &ret, true);
else else
ret = ip6_create_rt_rcu(rt); ret = ip6_create_rt_rcu(rt);
...@@ -3303,7 +3319,8 @@ static int ip6_route_del(struct fib6_config *cfg, ...@@ -3303,7 +3319,8 @@ static int ip6_route_del(struct fib6_config *cfg,
continue; continue;
if (cfg->fc_protocol && cfg->fc_protocol != rt->fib6_protocol) if (cfg->fc_protocol && cfg->fc_protocol != rt->fib6_protocol)
continue; continue;
fib6_info_hold(rt); if (!fib6_info_hold_safe(rt))
continue;
rcu_read_unlock(); rcu_read_unlock();
/* if gateway was specified only delete the one hop */ /* if gateway was specified only delete the one hop */
...@@ -3409,6 +3426,9 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu ...@@ -3409,6 +3426,9 @@ static void rt6_do_redirect(struct dst_entry *dst, struct sock *sk, struct sk_bu
rcu_read_lock(); rcu_read_lock();
from = rcu_dereference(rt->from); from = rcu_dereference(rt->from);
/* This fib6_info_hold() is safe here because we hold reference to rt
* and rt already holds reference to fib6_info.
*/
fib6_info_hold(from); fib6_info_hold(from);
rcu_read_unlock(); rcu_read_unlock();
...@@ -3470,7 +3490,8 @@ static struct fib6_info *rt6_get_route_info(struct net *net, ...@@ -3470,7 +3490,8 @@ static struct fib6_info *rt6_get_route_info(struct net *net,
continue; continue;
if (!ipv6_addr_equal(&rt->fib6_nh.nh_gw, gwaddr)) if (!ipv6_addr_equal(&rt->fib6_nh.nh_gw, gwaddr))
continue; continue;
fib6_info_hold(rt); if (!fib6_info_hold_safe(rt))
continue;
break; break;
} }
out: out:
...@@ -3530,8 +3551,8 @@ struct fib6_info *rt6_get_dflt_router(struct net *net, ...@@ -3530,8 +3551,8 @@ struct fib6_info *rt6_get_dflt_router(struct net *net,
ipv6_addr_equal(&rt->fib6_nh.nh_gw, addr)) ipv6_addr_equal(&rt->fib6_nh.nh_gw, addr))
break; break;
} }
if (rt) if (rt && !fib6_info_hold_safe(rt))
fib6_info_hold(rt); rt = NULL;
rcu_read_unlock(); rcu_read_unlock();
return rt; return rt;
} }
...@@ -3579,8 +3600,8 @@ static void __rt6_purge_dflt_routers(struct net *net, ...@@ -3579,8 +3600,8 @@ static void __rt6_purge_dflt_routers(struct net *net,
struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL; struct inet6_dev *idev = dev ? __in6_dev_get(dev) : NULL;
if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) && if (rt->fib6_flags & (RTF_DEFAULT | RTF_ADDRCONF) &&
(!idev || idev->cnf.accept_ra != 2)) { (!idev || idev->cnf.accept_ra != 2) &&
fib6_info_hold(rt); fib6_info_hold_safe(rt)) {
rcu_read_unlock(); rcu_read_unlock();
ip6_del_rt(net, rt); ip6_del_rt(net, rt);
goto restart; goto restart;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment