Re: [PATCH net] udp: do not use a cross-family sk_rx_dst in early demux

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message says "TCP does not have the same problem",
  but tcp_v6_early_demux() (net/ipv6/ip6_input.c) uses the same pattern…

--

udp: do not use a cross-family sk_rx_dst in early demux

With this patch, udp_v4_early_demux() and udp_v6_early_demux() use the
cached sk->sk_rx_dst only when dst->ops->family matches the packet's
family. Without the check, a dual-stack socket connected to a v4-mapped
address can attach an IPv4 dst to an IPv6 skb, or the reverse, because
the dst_check() callbacks can ignore the cookie.

> TCP does not have the same problem: sk_ehashfn() hashes a v4-mapped
> socket with inet_ehashfn(), while IPv6 lookups use inet6_ehashfn(),
> so IPv6 packets do not normally find a v4-mapped TCP socket.

[Severity: Medium]
Is this statement about TCP fully accurate? tcp_v6_early_demux() in
net/ipv6/ip6_input.c uses the same pattern this patch removes from UDP.
It never checks the family of the cached dst:

net/ipv6/ip6_input.c:tcp_v6_early_demux() {
    ...
		if (sk_fullsock(sk)) {
			struct dst_entry *dst = rcu_dereference(sk->sk_rx_dst);

			if (dst)
				dst = dst_check(dst, sk->sk_rx_dst_cookie);
			if (dst &&
			    sk->sk_rx_dst_ifindex == skb->skb_iif)
				skb_dst_set_noref(skb, dst);
		}
    ...
}

A v4-mapped AF_INET6 TCP socket caches an IPv4 rtable in sk_rx_dst.
That covers a dual-stack child accepted over IPv4 and a socket connected
to ::ffff:a.b.c.d. ipv4_dst_check() ignores the cookie, so dst_check()
here does not reject that rtable.

Take a spoofed IPv6 segment from ::ffff:C to ::ffff:S with matching ports
(ip6_rcv_core() now accepts it). inet6_match() would accept the
v4-mapped socket for it. The only thing that keeps the socket out of the
lookup is the sk_hash comparison:

net/ipv6/inet6_hashtables.c:__inet6_lookup_established() {
    ...
	hash = inet6_ehashfn(net, daddr, hnum, saddr, sport);
    ...
		if (sk->sk_hash != hash)
			continue;
		if (!inet6_match(net, sk, saddr, daddr, ports, dif, sdif))
			continue;
    ...
}

The socket's sk_hash comes from inet_ehashfn() in sk_ehashfn(), keyed by
inet_ehash_secret. The lookup uses inet6_ehashfn(), keyed by
tcp_ipv6_hash_secret.

These are two independently keyed 32-bit hashes. A collision for a given
tuple is unlikely but possible. The secrets are fixed for each boot, so a
tuple that collides once keeps colliding.

If such a collision happens, the IPv4 dst would be attached to the IPv6
skb. Then ip6_rcv_finish_core() would skip ip6_route_input(), and
ip_local_deliver() would parse the IPv6 header as IPv4. Is that the same
misparse this patch fixes for UDP?

The XFRM non-refcounted socket issue does not apply here, because
__inet6_lookup_established() takes a reference and the destructor is
sock_edemux.

Could the same family guard go into tcp_v6_early_demux() (AF_INET6) and
tcp_v4_early_demux() (AF_INET)? Otherwise, the commit message could say
that TCP relies on the hash mismatch rather than on an explicit check.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106110135.2161204-1-edumazet%40kernel.org