netdev - Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

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: <Z7ysHMi4NociwDgR@debian>
Date: Mon, 24 Feb 2025 18:27:56 +0100
From: Guillaume Nault <gnault@...hat.com>
To: Ido Schimmel <idosch@...sch.org>
Cc: David Miller <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	netdev@...r.kernel.org, Simon Horman <horms@...nel.org>,
	David Ahern <dsahern@...nel.org>,
	Antonio Quartulli <antonio@...delbit.com>
Subject: Re: [PATCH net v2 1/2] gre: Fix IPv6 link-local address generation.

On Sun, Feb 23, 2025 at 03:16:08PM +0200, Ido Schimmel wrote:
> On Fri, Feb 21, 2025 at 10:24:04AM +0100, Guillaume Nault wrote:
> > Use addrconf_addr_gen() to generate IPv6 link-local addresses on GRE
> > devices in most cases and fall back to using add_v4_addrs() only in
> > case the GRE configuration is incompatible with addrconf_addr_gen().
> > 
> > GRE used to use addrconf_addr_gen() until commit e5dd729460ca
> > ("ip/ip6_gre: use the same logic as SIT interfaces when computing v6LL
> > address") restricted this use to gretap devices and created
> 
> It's not always clear throughout the commit message to which devices you
> are referring to.

Yes, that's a problem I had when writing the commit message: I couldn't
find a proper way to name the different GRE device types unambiguously.

By reusing the device types of "ip link" we don't know if "gre" refers
to all GRE types or if it's only for IPv4 encapsulation. But using the
ARPHRD_* types wouldn't help, as that wouldn't allow to distinguish
between gretap and ip6gretap.

Maybe the following terms would be clearer:
'ip4gre', 'ip4gretap', 'ip6gre', 'ip6gretap' (and potentially 'ipXgre'
and 'ipXgretap' when considering both the IPv4 and IPv6 tunnel
versions). Would you find these terms clearer?

> For example, here, by "gretap" you mean both "gretap"
> and "ip6gretap", no?

Yes.

> BTW, I believe the check against 'ARPHRD_ETHER' in addrconf_gre_config()
> is dead code. addrconf_gre_config() is only called for ARPHRD_IP{,6}GRE
> devices.

Yes, that was dead code. But I'm reusing that condition to minimise
code changes so to make the fix simpler. Do you mean I should write
explicitely, somewhere, that it was dead code but isn't anymore?

> > add_v4_addrs() (borrowed from SIT) for non-Ethernet GRE ones.
> > 
> > The original problem came when commit 9af28511be10 ("addrconf: refuse
> > isatap eui64 for INADDR_ANY") made __ipv6_isatap_ifid() fail when its
> > addr parameter was 0. The commit says that this would create an invalid
> > address, however, I couldn't find any RFC saying that the generated
> > interface identifier would be wrong. Anyway, since plain gre devices
> > pass their local tunnel address to __ipv6_isatap_ifid(), that commit
> > broke their IPv6 link-local address generation when the local address
> > was unspecified.
> 
> By "plain gre devices" you mean "ipgre"? Because addrconf_ifid_ip6tnl()
> is called for "ip6gre" and it doesn't fail, unlike __ipv6_isatap_ifid().

Exactly. I tried to use the "plain" adjective to say that's the kind of
device you get with the "gre" keyword in "ip link".

> > Then commit e5dd729460ca ("ip/ip6_gre: use the same logic as SIT
> > interfaces when computing v6LL address") tried to fix that case by
> > defining add_v4_addrs() and calling it to generated the IPv6 link-local
> 
> s/generated/generate/
> 
> > address instead of using addrconf_addr_gen() (appart for gretap devices
> 
> s/appart/apart/

Will fix both.

> > which would still use the regular addrconf_addr_gen(), since they have
> > a MAC address).
> 
> Assuming what I wrote is correct, I'm not sure why e5dd729460ca didn't
> restrict the fix to "ipgre" and applied it to "ip6gre" as well.

I asked myself the same question. Antonio might have an answer to this.
But in my understanding the changes brought by e5dd729460ca were much too
broad.

> > -	if (dev->type == ARPHRD_ETHER) {
> > +	/* Generate the IPv6 link-local address using addrconf_addr_gen(),
> > +	 * unless we have an IPv4 GRE device not bound to an IP address and
> > +	 * which is in EUI64 mode (as __ipv6_isatap_ifid() would fail in this
> > +	 * case). Such devices fall back to add_v4_addrs() instead.
> > +	 */
> > +	if (!(dev->type == ARPHRD_IPGRE && *(__be32 *)dev->dev_addr == 0 &&
> 
> Doesn't this mean that the 'ARPHRD_IP6GRE' case (and the
> 'CONFIG_IPV6_GRE' checks) can be removed from
> addrconf_init_auto_addrs()? That is, only call addrconf_gre_config() for
> "ipgre", but not for "ip6gre".

Yes. But I didn't want to do that here, to keep the fix as simple as
possible. Because that'd mean we'd also have to add a
"(dev->type != ARPHRD_IP6GRE)" condition in the test at the beginning
of addrconf_dev_config(), and I feel that'd be a distraction from the
core of the patch. So I prefer to do that in net-next.

> > +	      idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64)) {
> >  		addrconf_addr_gen(idev, true);
> >  		return;
> >  	}
> > -- 
> > 2.39.2
> > 
> > 
> 

Thanks a lot for your review.