netdev - Re: [PATCH net] ipv4: reject RTNH_F_LINKDOWN for incompatible routes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160709172327.GP18787@gospo.rdu.cumulusnetworks.com>
Date:	Sat, 9 Jul 2016 13:23:28 -0400
From:	Andy Gospodarek <gospo@...ulusnetworks.com>
To:	Julian Anastasov <ja@....bg>
Cc:	David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
	Vegard Nossum <vegard.nossum@...cle.com>,
	Dinesh Dutt <ddutt@...ulusnetworks.com>,
	Scott Feldman <sfeldma@...il.com>
Subject: 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 <vegard.nossum@...cle.com>
> Fixes: 0eeb075fad73 ("net: ipv4 sysctl option to ignore routes when nexthop link is down")
> Tested-by: Vegard Nossum <vegard.nossum@...cle.com>
> Signed-off-by: Julian Anastasov <ja@....bg>
> Cc: Andy Gospodarek <gospo@...ulusnetworks.com>
> Cc: Dinesh Dutt <ddutt@...ulusnetworks.com>
> Cc: Scott Feldman <sfeldma@...il.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
>