netdev - Re: [PATCHv5 net-next] ipv6: do not match device when remove source route

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: <ZNkASnjqmAVg2vBg@shredder>
Date: Sun, 13 Aug 2023 19:09:46 +0300
From: Ido Schimmel <idosch@...sch.org>
To: Hangbin Liu <liuhangbin@...il.com>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
	David Ahern <dsahern@...nel.org>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Thomas Haller <thaller@...hat.com>
Subject: Re: [PATCHv5 net-next] ipv6: do not match device when remove source
 route

On Fri, Aug 11, 2023 at 05:53:08PM +0800, Hangbin Liu wrote:
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 64e873f5895f..0f981cc5bed1 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -4590,11 +4590,12 @@ static int fib6_remove_prefsrc(struct fib6_info *rt, void *arg)
>  	struct net_device *dev = ((struct arg_dev_net_ip *)arg)->dev;
>  	struct net *net = ((struct arg_dev_net_ip *)arg)->net;
>  	struct in6_addr *addr = ((struct arg_dev_net_ip *)arg)->addr;
> +	u32 tb6_id = l3mdev_fib_table(dev) ? : RT_TABLE_MAIN;
>  
> -	if (!rt->nh &&
> -	    ((void *)rt->fib6_nh->fib_nh_dev == dev || !dev) &&
> -	    rt != net->ipv6.fib6_null_entry &&
> -	    ipv6_addr_equal(addr, &rt->fib6_prefsrc.addr)) {
> +	if (rt != net->ipv6.fib6_null_entry &&
> +	    rt->fib6_table->tb6_id == tb6_id &&
> +	    ipv6_addr_equal(addr, &rt->fib6_prefsrc.addr) &&
> +	    !ipv6_chk_addr(net, addr, rt->fib6_nh->fib_nh_dev, 0)) {
>  		spin_lock_bh(&rt6_exception_lock);
>  		/* remove prefsrc entry */
>  		rt->fib6_prefsrc.plen = 0;

The table check is incorrect which is what I was trying to explain here
[1]. The route insertion code does not check that the preferred source
is accessible from the VRF where the route is installed, but instead
that it is accessible from the VRF of the first nexthop device. I'm not
going to debate whether it is correct or not. I'm going to say that the
logic should be consistent between the route insertion and deletion
paths. That is, if I'm only able to insert a route with a preferred
source address because some address exists, then when this address is
removed the preferred source address should be removed from the route.

Here is an example with your patch applied:

+ ip link add name dummy1 up type dummy
+ ip link add name vrf1 up type vrf table 1111
+ ip link set dev dummy1 master vrf1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
Error: Invalid source address.
+ ip address add 2001:db8:1::1/64 dev dummy1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium
+ ip address del 2001:db8:1::1/64 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium

Note how it is not possible to add the route to the main table because
the address does not exist, but then after the address is deleted the
route still exists with the preferred source address.

And this is the same example, but with the patch from [1]:

+ ip link add name dummy1 up type dummy
+ ip link add name vrf1 up type vrf table 1111
+ ip link set dev dummy1 master vrf1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
Error: Invalid source address.
+ ip address add 2001:db8:1::1/64 dev dummy1
+ ip -6 route add 2001:db8:2::/64 src 2001:db8:1::1 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 src 2001:db8:1::1 metric 1024 pref medium
+ ip address del 2001:db8:1::1/64 dev dummy1
+ ip -6 route show 2001:db8:2::/64
2001:db8:2::/64 dev dummy1 metric 1024 pref medium

[1] https://lore.kernel.org/netdev/ZNSol%2F7x5oI6amEB@shredder/