diff mbox

[net] ipv4: reject RTNH_F_LINKDOWN for incompatible routes

Message ID 1468054815-24766-1-git-send-email-ja@ssi.bg
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Julian Anastasov July 9, 2016, 9 a.m. UTC 
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.

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.

Reported-by: Vegard Nossum <vegard.nossum@oracle.com>
Fixes: 0eeb075fad73 ("net: ipv4 sysctl option to ignore routes when nexthop link is down")
Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
Signed-off-by: Julian Anastasov <ja@ssi.bg>
Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
Cc: Dinesh Dutt <ddutt@cumulusnetworks.com>
Cc: Scott Feldman <sfeldma@gmail.com>
---
 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

Comments

Andy Gospodarek July 9, 2016, 5:23 p.m. UTC | #1 
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 <vegard.nossum@oracle.com>
> Fixes: 0eeb075fad73 ("net: ipv4 sysctl option to ignore routes when nexthop link is down")
> Tested-by: Vegard Nossum <vegard.nossum@oracle.com>
> Signed-off-by: Julian Anastasov <ja@ssi.bg>
> Cc: Andy Gospodarek <gospo@cumulusnetworks.com>
> Cc: Dinesh Dutt <ddutt@cumulusnetworks.com>
> Cc: Scott Feldman <sfeldma@gmail.com>
> ---
>  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
>
Vegard Nossum July 9, 2016, 7:10 p.m. UTC | #2 
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:[<00000000602b3d18>]
RSP: 0000000062623890  EFLAGS: 00010202
RAX: 0000000000000000 RBX: 000000006261b800 RCX: 0000000000000000
RDX: 0000000000000000 RSI: 0000000000000024 RDI: 000000006245ba00
RBP: 00000000626238f0 R08: 000000000000029c R09: 0000000000000000
R10: 0000000062468038 R11: 000000006245ba00 R12: 000000006245ba00
R13: 00000000625f96c0 R14: 00000000601e16f0 R15: 0000000000000000
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 00000400 000000fe
  62623910 600afca7 62623970 62623a48
  62468038 00000018 00000000 00000000
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
Julian Anastasov July 9, 2016, 8:11 p.m. UTC | #3 
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 <ja@ssi.bg>
diff mbox

Patch

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;
 		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);