netdev - Re: [PATCH net-next v2 3/4] nexthop: Fix out-of-bounds access during attribute validation

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: <2a064df1-9c43-4224-aa35-6c7939852d1b@kernel.org>
Date: Mon, 11 Mar 2024 21:28:30 -0600
From: David Ahern <dsahern@...nel.org>
To: Ido Schimmel <idosch@...dia.com>, netdev@...r.kernel.org
Cc: davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com,
 edumazet@...gle.com, petrm@...dia.com
Subject: Re: [PATCH net-next v2 3/4] nexthop: Fix out-of-bounds access during
 attribute validation

On 3/11/24 10:23 AM, Ido Schimmel wrote:
> Passing a maximum attribute type to nlmsg_parse() that is larger than
> the size of the passed policy will result in an out-of-bounds access [1]
> when the attribute type is used as an index into the policy array.
> 
> Fix by setting the maximum attribute type according to the policy size,
> as is already done for RTM_NEWNEXTHOP messages. Add a test case that
> triggers the bug.
> 
> No regressions in fib nexthops tests:
> 
>  # ./fib_nexthops.sh
>  [...]
>  Tests passed: 236
>  Tests failed:   0
> 
> [1]
> BUG: KASAN: global-out-of-bounds in __nla_validate_parse+0x1e53/0x2940
> Read of size 1 at addr ffffffff99ab4d20 by task ip/610
> 
> CPU: 3 PID: 610 Comm: ip Not tainted 6.8.0-rc7-custom-gd435d6e3e161 #9
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.2-1.fc38 04/01/2014
> Call Trace:
>  <TASK>
>  dump_stack_lvl+0x8f/0xe0
>  print_report+0xcf/0x670
>  kasan_report+0xd8/0x110
>  __nla_validate_parse+0x1e53/0x2940
>  __nla_parse+0x40/0x50
>  rtm_del_nexthop+0x1bd/0x400
>  rtnetlink_rcv_msg+0x3cc/0xf20
>  netlink_rcv_skb+0x170/0x440
>  netlink_unicast+0x540/0x820
>  netlink_sendmsg+0x8d3/0xdb0
>  ____sys_sendmsg+0x31f/0xa60
>  ___sys_sendmsg+0x13a/0x1e0
>  __sys_sendmsg+0x11c/0x1f0
>  do_syscall_64+0xc5/0x1d0
>  entry_SYSCALL_64_after_hwframe+0x63/0x6b
> [...]
> 
> The buggy address belongs to the variable:
>  rtm_nh_policy_del+0x20/0x40
> 
> Fixes: 2118f9390d83 ("net: nexthop: Adjust netlink policy parsing for a new attribute")
> Reported-by: Eric Dumazet <edumazet@...gle.com>
> Closes: https://lore.kernel.org/netdev/CANn89i+UNcG0PJMW5X7gOMunF38ryMh=L1aeZUKH3kL4UdUqag@mail.gmail.com/
> Reported-by: syzbot+65bb09a7208ce3d4a633@...kaller.appspotmail.com
> Closes: https://lore.kernel.org/netdev/00000000000088981b06133bc07b@google.com/
> Signed-off-by: Ido Schimmel <idosch@...dia.com>
> ---
> 
> Notes:
>     v2:
>     * Resize 'tb' using ARRAY_SIZE
> 
>  net/ipv4/nexthop.c                          | 29 ++++++++++++---------
>  tools/testing/selftests/net/fib_nexthops.sh |  6 +++++
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 

Reviewed-by: David Ahern <dsahern@...nel.org>


> diff --git a/tools/testing/selftests/net/fib_nexthops.sh b/tools/testing/selftests/net/fib_nexthops.sh
> index d5a281aadbac..ac0b2c6a5761 100755
> --- a/tools/testing/selftests/net/fib_nexthops.sh
> +++ b/tools/testing/selftests/net/fib_nexthops.sh
> @@ -2066,6 +2066,12 @@ basic()
>  	run_cmd "$IP nexthop get id 1"
>  	log_test $? 2 "Nexthop get on non-existent id"
>  
> +	run_cmd "$IP nexthop del id 1"
> +	log_test $? 2 "Nexthop del with non-existent id"
> +
> +	run_cmd "$IP nexthop del id 1 group 1/2/3/4/5/6/7/8"
> +	log_test $? 2 "Nexthop del with non-existent id and extra attributes"
> +
>  	# attempt to create nh without a device or gw - fails
>  	run_cmd "$IP nexthop add id 1"
>  	log_test $? 2 "Nexthop with no device or gateway"

The basic() group of tests do not have a delete, so this is a good
addition. However, the ipv6_fcnal and ipv4_fcnal do have a del - seems
like those tests should have caught the out of bounds access.