Re: [net,v3] udp: revalidate socket family before publishing an IPv6 cork

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 2 · Medium: 0 · Low: 0

- [High] Incomplete fix: the sibling IPv6 socket-dst publisher in the
  connect() path is not covered, so an IPv6 dst can still be installed…
- [High] The fix is writer-side and after-the-fact, so the mixed-family
  socket dst it claims to eliminate remains observable by the unguarded…

--

udp: revalidate socket family before publishing an IPv6 cork

This adds a family recheck after the connected IPv6 route lookup in
udpv6_sendmsg(), another one after lock_sock() before the cork is
published, and a sk_dst_reset() in the UDP IPV6_ADDRFORM path once
PF_INET has been written.  The goal is to stop an AF_INET6 cork or an
IPv6 socket dst from surviving on a socket converted to AF_INET.

Closes: https://lore.kernel.org/netdev/20260825160630.1888866-1-4ncienth@gmail.com/
Link: https://lore.kernel.org/netdev/a86aa34a-8bea-46d2-a785-6c423ac00b1d@redhat.com/

A couple of questions about the two orderings the changelog says are
closed.

> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index b4c977434c2e0..59ca446d3156b 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -611,6 +611,7 @@ int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>  				WRITE_ONCE(sk->sk_prot, &udp_prot);
>  				WRITE_ONCE(sk->sk_socket->ops, &inet_dgram_ops);
>  				WRITE_ONCE(sk->sk_family, PF_INET);
> +				sk_dst_reset(sk);
>  			}

[Severity: High]
Can an IPv6 socket dst still be published after this reset by the
connect() path, which has no family revalidation?

inet_dgram_connect() in net/ipv4/af_inet.c resolves the protocol before
it can block, and says so:

	/* IPV6_ADDRFORM can change sk->sk_prot under us. */
	prot = READ_ONCE(sk->sk_prot);
	...
	return prot->connect(sk, uaddr, addr_len);

So a connect() that already dispatched to udpv6_connect() sits in
lock_sock() while do_ipv6_setsockopt() performs the conversion above and
drops the lock.

__ip6_datagram_connect() in net/ipv6/datagram.c then resumes and checks
only the address family supplied by userspace and ipv6_only_sock(); it
never re-reads sk->sk_family after the lock was taken:

	if (usin->sin6_family != AF_INET6)
		return -EAFNOSUPPORT;
	...
	err = ip6_datagram_dst_update(sk, true);

and ip6_datagram_dst_update() ends with:

	ip6_sk_dst_store_flow(sk, dst, fl6);

That installs an rt6_info into sk->sk_dst_cache on a socket that is
already AF_INET/udp_prot, i.e. after the new sk_dst_reset() has run.  The
IPv4 consumer then takes it without any family test, in udp_sendmsg():

	if (connected)
		rt = dst_rtable(sk_dst_check(sk, 0));

Note the cookie 0 does not reject it for every route type, since
ip6_dst_check() short-circuits:

	if (rt->sernum)
		return rt6_is_valid(rt) ? dst : NULL;

and rt_genid_ipv6() is seeded from fib6_sernum, which starts at 1, so
pcpu routes of nexthop-object fib entries always carry a nonzero sernum.

Given that, is the claim in the changelog accurate?

  "Thus, if the send stores and checks first, the later conversion clears
  the cache; if conversion and its reset happen first, the send observes
  AF_INET after its store and clears it."

The reset here only orders against stores made by code that revalidates
the family; would a family recheck in __ip6_datagram_connect() after
lock_sock(), or a dst->ops->family check on the IPv4 consumer side, be
needed as well?

> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index 93478d1ad5769..8ba552f1a6b26 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1705,6 +1705,12 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  		dst = NULL;
>  		goto out;
>  	}
> +	if (unlikely(connected &&
> +		     READ_ONCE(sk->sk_family) != AF_INET6)) {
> +		sk_dst_reset(sk);
> +		err = -EAFNOSUPPORT;
> +		goto out;
> +	}

[Severity: High]
Is the mixed-family socket dst observable to a third thread in the window
between the store and this check?

The store happens inside the lookup just above, in
ip6_sk_dst_lookup_flow():

	dst = ip6_dst_lookup_flow(sock_net(sk), sk, fl6, final_dst);
	if (connected && !IS_ERR(dst))
		ip6_sk_dst_store_flow(sk, dst_clone(dst), fl6);

so in the "conversion wins first" ordering the sequence is:

CPU1 (setsockopt IPV6_ADDRFORM)
	WRITE_ONCE(sk->sk_family, PF_INET);
	sk_dst_reset(sk);

CPU0 (udpv6_sendmsg, connected == true)
	ip6_sk_dst_store_flow(sk, ...);	/* rt6_info now cached */
	<preemptible window, no socket lock held>
	READ_ONCE(sk->sk_family) != AF_INET6 -> sk_dst_reset(sk);

CPU2 (udp_sendmsg on the converted socket)
	rt = dst_rtable(sk_dst_check(sk, 0));

The IPv4 side has no family validation, unlike its IPv6 counterpart
ip6_sk_dst_check() which rejects a foreign entry outright:

	if (dst->ops->family != AF_INET6) {
		dst_release(dst);
		return NULL;
	}

Does that leave an rt6_info being used as a struct rtable, with an
IPv4-formatted skb reaching dst->output == ip6_output()?

The symmetric ordering also looks reachable: in do_ipv6_setsockopt() the
udp_prot, inet_dgram_ops and PF_INET writes are published before the new
sk_dst_reset(), and inet6_sendmsg() dispatches on the lockless read:

	/* IPV6_ADDRFORM can change sk->sk_prot under us. */
	prot = READ_ONCE(sk->sk_prot);

Can a thread therefore enter IPv4 udp_sendmsg() and pick up the still
cached IPv6 dst before the reset runs, in which case the reset cannot
revoke the reference it already took?

Would validating the family (or performing the connected store) under
lock_sock, or adding a dst->ops->family check to the IPv4 path, cover
these two windows instead of the after-the-fact reset?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902010408.1057857-1-4ncienth%40gmail.com