Re: [PATCH net] ipv4: reject RTNH_F_LINKDOWN for incompatible routes
Hello, On Sat, 9 Jul 2016, Andy Gospodarek wrote: > On Sat, Jul 09, 2016 at 12:00:15PM +0300, Julian Anastasov wrote: > > Vegard Nossum is reporting for a crash in fib_dump_info (fib_nhs==1) > > when nh_dev = NULL. Problem happens when RTNH_F_LINKDOWN is > > provided from user space for routes that do not use the flag, > > catched with netlink fuzzer. > > Can you also include the panic log in the changelog or at a minimum post > it here? Now after Vegard posted it, I'll include in v2. > > RTNH_F_LINKDOWN should be used only for link routes, not for > > local routes or for routes with error code. Do not complicate > > fast path with more checks, reject the flag early when configured > > for incompatible routes. > > Did the netlink fuzzer (trinity?) happen to check any of the other flags > (liks RTNH_F_DEAD) that are normally set by the kernel but could be > problematic when send down from userspace? My guess is that fib_flush will release it soon or later. I preferred to reject the RTNH_F_LINKDOWN flag only for some kind of routes but another alternative is to always reject both RTNH_F_LINKDOWN and RTNH_F_DEAD: RTNH_F_LINKDOWN is recalculated and there is no good reason user space to provide initial value for flag that is maintained by kernel. > > if (fib_props[cfg->fc_type].error) { > > - if (cfg->fc_gw || cfg->fc_oif || cfg->fc_mp) > > + if (cfg->fc_gw || cfg->fc_oif || cfg->fc_mp || > > + (fi->fib_nh->nh_flags & RTNH_F_LINKDOWN)) > > goto err_inval; > > It looks a bit odd to use cfg in the existing checkd and fi->fib_nh in > the rest, but not a huge issue since cfg->fc_flags and > fi->fib_nh->nh_flags should be equivalent should be the same for single > and multipath routes. Using fc_flags works too for the above case. In fact, it goes also to fib_flags, so we should have our checks there. But it is true that RTNH_F_LINKDOWN is not used from fib_flags, I think, we already had a chance to talk about it on 27 Oct 2015. May be we can reject the both flags once for rtnh_flags in fib_get_nhs() and then for fc_flags in fib_create_info(). Regards -- Julian Anastasov
Re: [PATCH net] ipv4: reject RTNH_F_LINKDOWN for incompatible routes
On 07/09/2016 07:23 PM, Andy Gospodarek wrote: On Sat, Jul 09, 2016 at 12:00:15PM +0300, Julian Anastasov wrote: Vegard Nossum is reporting for a crash in fib_dump_info (fib_nhs==1) when nh_dev = NULL. Problem happens when RTNH_F_LINKDOWN is provided from user space for routes that do not use the flag, catched with netlink fuzzer. Can you also include the panic log in the changelog or at a minimum post it here? Pid: 50, comm: netlink.exe Not tainted 4.7.0-rc5+ RIP: 0033:[<602b3d18>] RSP: 62623890 EFLAGS: 00010202 RAX: RBX: 6261b800 RCX: RDX: RSI: 0024 RDI: 6245ba00 RBP: 626238f0 R08: 029c R09: R10: 62468038 R11: 6245ba00 R12: 6245ba00 R13: 625f96c0 R14: 601e16f0 R15: Kernel panic - not syncing: Kernel mode fault at addr 0x2e0, ip 0x602b3d18 CPU: 0 PID: 50 Comm: netlink.exe Not tainted 4.7.0-rc5+ #581 Stack: 626238f0 960226a02 0400 00fe 62623910 600afca7 62623970 62623a48 62468038 0018 Call Trace: [<602b3e93>] rtmsg_fib+0xd3/0x190 [<602b6680>] fib_table_insert+0x260/0x500 [<602b0e5d>] inet_rtm_newroute+0x4d/0x60 [<60250def>] rtnetlink_rcv_msg+0x8f/0x270 [<60267079>] netlink_rcv_skb+0xc9/0xe0 [<60250d4b>] rtnetlink_rcv+0x3b/0x50 [<60265400>] netlink_unicast+0x1a0/0x2c0 [<60265e47>] netlink_sendmsg+0x3f7/0x470 [<6021dc9a>] sock_sendmsg+0x3a/0x90 [<6021e0d0>] ___sys_sendmsg+0x300/0x360 [<6021fa64>] __sys_sendmsg+0x54/0xa0 [<6021fac0>] SyS_sendmsg+0x10/0x20 [<6001ea68>] handle_syscall+0x88/0x90 [<600295fd>] userspace+0x3fd/0x500 [<6001ac55>] fork_handler+0x85/0x90 $ addr2line -e vmlinux -i 0x602b3d18 include/linux/inetdevice.h:222 net/ipv4/fib_semantics.c:1264 220 static inline struct in_device *__in_dev_get_rtnl(const struct net_device *dev) 221 { 222 return rtnl_dereference(dev->ip_ptr); 223 } 1263 if (fi->fib_nh->nh_flags & RTNH_F_LINKDOWN) { 1264 in_dev = __in_dev_get_rtnl(fi->fib_nh->nh_dev); 1265 if (in_dev && RTNH_F_LINKDOWN should be used only for link routes, not for local routes or for routes with error code. Do not complicate fast path with more checks, reject the flag early when configured for incompatible routes. Did the netlink fuzzer (trinity?) happen to check any of the other flags (liks RTNH_F_DEAD) that are normally set by the kernel but could be problematic when send down from userspace? I honestly don't know -- the fuzzer (based on AFL) doesn't know anything about netlink in particular, so if it passed/tested any other flags it was by chance and not by design. Vegard
Re: [PATCH net] ipv4: reject RTNH_F_LINKDOWN for incompatible routes
On Sat, Jul 09, 2016 at 12:00:15PM +0300, Julian Anastasov wrote: > Vegard Nossum is reporting for a crash in fib_dump_info (fib_nhs==1) > when nh_dev = NULL. Problem happens when RTNH_F_LINKDOWN is > provided from user space for routes that do not use the flag, > catched with netlink fuzzer. Can you also include the panic log in the changelog or at a minimum post it here? > RTNH_F_LINKDOWN should be used only for link routes, not for > local routes or for routes with error code. Do not complicate > fast path with more checks, reject the flag early when configured > for incompatible routes. Did the netlink fuzzer (trinity?) happen to check any of the other flags (liks RTNH_F_DEAD) that are normally set by the kernel but could be problematic when send down from userspace? > Reported-by: Vegard Nossum> Fixes: 0eeb075fad73 ("net: ipv4 sysctl option to ignore routes when nexthop > link is down") > Tested-by: Vegard Nossum > Signed-off-by: Julian Anastasov > Cc: Andy Gospodarek > Cc: Dinesh Dutt > Cc: Scott Feldman > --- > net/ipv4/fib_semantics.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > Note: works for all kernels: net, net-next, 4.4.14, 4.5.7, 4.6.3 > > diff --git a/net/ipv4/fib_semantics.c b/net/ipv4/fib_semantics.c > index d09173b..b642479 100644 > --- a/net/ipv4/fib_semantics.c > +++ b/net/ipv4/fib_semantics.c > @@ -1113,7 +1113,8 @@ struct fib_info *fib_create_info(struct fib_config *cfg) > } > > if (fib_props[cfg->fc_type].error) { > - if (cfg->fc_gw || cfg->fc_oif || cfg->fc_mp) > + if (cfg->fc_gw || cfg->fc_oif || cfg->fc_mp || > + (fi->fib_nh->nh_flags & RTNH_F_LINKDOWN)) > goto err_inval; It looks a bit odd to use cfg in the existing checkd and fi->fib_nh in the rest, but not a huge issue since cfg->fc_flags and fi->fib_nh->nh_flags should be equivalent should be the same for single and multipath routes. > goto link_it; > } else { > @@ -1136,7 +1137,7 @@ struct fib_info *fib_create_info(struct fib_config *cfg) > struct fib_nh *nh = fi->fib_nh; > > /* Local address is added. */ > - if (nhs != 1 || nh->nh_gw) > + if (nhs != 1 || nh->nh_gw || (nh->nh_flags & RTNH_F_LINKDOWN)) > goto err_inval; > nh->nh_scope = RT_SCOPE_NOWHERE; > nh->nh_dev = dev_get_by_index(net, fi->fib_nh->nh_oif); > -- > 1.9.3 >