netdev - Re: [PATCHv2 net] ipv4: reject RTNH_F_DEAD and RTNH_F

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: <20160711133606.GQ18787@gospo.rdu.cumulusnetworks.com>
Date:	Mon, 11 Jul 2016 09:36:07 -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: [PATCHv2 net] ipv4: reject RTNH_F_DEAD and RTNH_F_LINKDOWN from
 user space

On Sun, Jul 10, 2016 at 09:11:55PM +0300, Julian Anastasov wrote:
> Vegard Nossum is reporting for a crash in fib_dump_info
> when nh_dev = NULL and fib_nhs == 1:
> 
> 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
> 
> Problem happens when RTNH_F_LINKDOWN is provided from user space
> when creating routes that do not use the flag, catched with
> netlink fuzzer.
> 
> Currently, the kernel allows user space to set both flags
> to nh_flags and fib_flags but this is not intentional, the
> assumption was that they are not set. Fix this by rejecting
> both flags with EINVAL.
> 
> Reported-by: Vegard Nossum <vegard.nossum@...cle.com>
> Fixes: 0eeb075fad73 ("net: ipv4 sysctl option to ignore routes when nexthop link is down")
> 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>

It looks like this catches the single and multipath next-hop cases, so
looks good to me.

Reviewed-by: Andy Gospodarek <gospo@...ulusnetworks.com>

> ---
>  net/ipv4/fib_semantics.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> 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..539fa26 100644
> --- a/net/ipv4/fib_semantics.c
> +++ b/net/ipv4/fib_semantics.c
> @@ -479,6 +479,9 @@ static int fib_get_nhs(struct fib_info *fi, struct rtnexthop *rtnh,
>  		if (!rtnh_ok(rtnh, remaining))
>  			return -EINVAL;
>  
> +		if (rtnh->rtnh_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN))
> +			return -EINVAL;
> +
>  		nexthop_nh->nh_flags =
>  			(cfg->fc_flags & ~0xFF) | rtnh->rtnh_flags;
>  		nexthop_nh->nh_oif = rtnh->rtnh_ifindex;
> @@ -1003,6 +1006,9 @@ struct fib_info *fib_create_info(struct fib_config *cfg)
>  	if (fib_props[cfg->fc_type].scope > cfg->fc_scope)
>  		goto err_inval;
>  
> +	if (cfg->fc_flags & (RTNH_F_DEAD | RTNH_F_LINKDOWN))
> +		goto err_inval;
> +
>  #ifdef CONFIG_IP_ROUTE_MULTIPATH
>  	if (cfg->fc_mp) {
>  		nhs = fib_count_nexthops(cfg->fc_mp, cfg->fc_mp_len);
> -- 
> 1.9.3
>