netdev - Re: [PATCHv4 net] 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: <ZMtbK16VpqGwbws1@Laptop-X1>
Date: Thu, 3 Aug 2023 15:45:47 +0800
From: Hangbin Liu <liuhangbin@...il.com>
To: Ido Schimmel <idosch@...sch.org>
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: [PATCHv4 net] ipv6: do not match device when remove source route

On Tue, Aug 01, 2023 at 02:51:52PM +0300, Ido Schimmel wrote:
> So, I think we need to call ipv6_chk_addr() from rt6_remove_prefsrc() to
> be consistent with the addition path in ip6_route_info_create():
> 
> ipv6_chk_addr() is not cheap, but it's only called for routes that match
> the previous criteria.
> 
> There is however one failure in the selftest:
> 
> Which is basically:
> 
> # ip link add name dummy1 up type dummy
> # ip link add name dummy2 up type dummy
> # ip link add red type vrf table 1111
> # ip link set dev red up
> # ip link set dummy2 vrf red
> # ip -6 address add dev dummy1 2001:db8:104::12/64
> # ip -6 address add dev dummy2 2001:db8:104::12/64
> # ip -6 route add 2001:db8:106::/64 dev lo src 2001:db8:104::12
> # ip -6 route add vrf red 2001:db8:106::/64 dev lo src 2001:db8:104::12
> # ip -6 address del dev dummy2 2001:db8:104::12/64
> # ip -6 route show vrf red | grep "src 2001:db8:104::12"
> 2001:db8:106::/64 dev lo src 2001:db8:104::12 metric 1024 pref medium
> 
> I'm not sure it's realistic to expect the source address to be removed
> when the address is deleted from dummy2, given that user space was only
> able to configure the route because the address was available on dummy1
> in the default vrf:
> 
> # ip link add name dummy1 up type dummy
> # ip link add name dummy2 up type dummy
> # ip link add red type vrf table 1111
> # ip link set dev red up
> # ip link set dummy2 vrf red
> # ip -6 address add dev dummy2 2001:db8:104::12/64
> # ip -6 route add vrf red 2001:db8:106::/64 dev lo src 2001:db8:104::12
> Error: Invalid source address.

After looking into it. I think this looks like a bug in
ip6_route_info_create(). When the dev is lo. It's vrf is the default vrf
table, which makes ipv6_chk_addr() only check dummy1 instead of dummy2.
Maybe we should add a special check for lo dev?

On the other hand, for route delete. How about add a special check for link
local src addr add loopback dev? e.g.

diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index b45b33394f43..6dbfae99f811 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -4586,11 +4586,17 @@ 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)) {
+       bool skip_dev_check = !(ipv6_addr_type(addr) & IPV6_ADDR_LINKLOCAL);
+
+       if (!(rt->fib6_nh->fib_nh_dev->flags & IFF_LOOPBACK))
+               dev = rt->fib6_nh->fib_nh_dev;
+
+       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_and_flags(net, addr, dev, skip_dev_check, 0, IFA_F_TENTATIVE)) {
                spin_lock_bh(&rt6_exception_lock);
                /* remove prefsrc entry */
                rt->fib6_prefsrc.plen = 0;


With this, we can pass the fib_tests and your link local src route testing

Thanks
Hangbin