[PATCH net] udp: revalidate socket family before publishing an IPv6 cork

[PATCH net] udp: revalidate socket family before publishing an IPv6 cork

[Daehyeon Ko]

udpv6_sendmsg() prepares the IPv6 flow and route before taking the socket
lock when a datagram is corked.  IPV6_ADDRFORM takes the same lock, but it
can convert the socket to AF_INET while the send path is doing that
lockless preparation because no cork has been published yet.

If the conversion wins the race, udpv6_sendmsg() later publishes an
AF_INET6 cork on an AF_INET socket.  Uncorking through the IPv4 socket
operations then interprets the IPv6 cork as IPv4 state.  The IPv4
finalizer writes a 20-byte IPv4 header into the 40-byte IPv6 header
reservation while the retained IPv6 dst routes the skb through
ip6_output().  ip6_finish_output2() consequently consumes the unwritten
20-byte tail.

An unprivileged reproducer triggered the mixed state on 12 of 10,000
sockets.  KMSAN reported an uninitialized-value read in
ip6_finish_output2() on three fresh boots, with the allocation origin in
__alloc_skb() through __ip6_append_data().  The same process recovered the
20-byte region from the TX timestamp error queue; one of three fresh boots
contained recognizable stale heap data.

After taking the lock, revalidate that IPV6_ADDRFORM has not changed the
socket family before publishing the cork.  The existing error path releases
the prepared dst, flowlabel, and transmit-option references.  With this
change, the serialized controls retain their existing results and the
forbidden mixed state occurred zero times across 20,000 sockets.

Fixes: 03485f2adcde ("udpv6: Add lockless sendmsg() support")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Daehyeon Ko <4ncienth@gmail.com>
---
 net/ipv6/udp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
index fd875908ac0c66..566c634a5a5945 100644
--- a/net/ipv6/udp.c
+++ b/net/ipv6/udp.c
@@ -1716,6 +1716,11 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
 	}
 
 	lock_sock(sk);
+	if (unlikely(sk->sk_family != AF_INET6)) {
+		release_sock(sk);
+		err = -EAFNOSUPPORT;
+		goto out;
+	}
 	if (unlikely(up->pending)) {
 		/* The socket is already corked while preparing it. */
 		/* ... which is an evident application bug. --ANK */
-- 
2.54.0

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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email says "You have to repost this with appropriate people in the
CC list, before you do please check the following AI generated
review."
---
udp: revalidate socket family before publishing an IPv6 cork

Adds a recheck of sk->sk_family after lock_sock() in udpv6_sendmsg(), so a
socket converted to AF_INET by IPV6_ADDRFORM during the lockless flow/route
preparation no longer gets an AF_INET6 cork published on it.  The mismatch
path bails out with -EAFNOSUPPORT through the existing error label.

> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
> index fd875908ac0c6..566c634a5a594 100644
> --- a/net/ipv6/udp.c
> +++ b/net/ipv6/udp.c
> @@ -1716,6 +1716,11 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>  	}
>  
>  	lock_sock(sk);
> +	if (unlikely(sk->sk_family != AF_INET6)) {
> +		release_sock(sk);
> +		err = -EAFNOSUPPORT;
> +		goto out;
> +	}

[Severity: High]
Does this branch leave the IPv6 dst published in sk->sk_dst_cache?

The lockless preparation earlier in udpv6_sendmsg() calls
ip6_sk_dst_lookup_flow(sk, fl6, final_p, connected), and for a connected
send that function stores the route on the socket before the lock is taken:

net/ipv6/ip6_output.c:ip6_sk_dst_lookup_flow() {
	...
	if (connected && !IS_ERR(dst))
		ip6_sk_dst_store_flow(sk, dst_clone(dst), fl6);
	...
}

That store happens inside the same window the recheck is meant to cover.
The new mismatch path then does release_sock(sk) and goto out, and out:
only drops the function's local reference with dst_release(dst).  Neither
do_ipv6_setsockopt(IPV6_ADDRFORM) nor inet6_cleanup_sock() calls
sk_dst_reset() - inet6_cleanup_sock() only frees pktoptions, rxpmtu,
flowlabels and np->opt - so the converted AF_INET socket appears to keep an
rt6_info in its dst cache.

Can the IPv4 path then consume that entry as an rtable?  udp_sendmsg() does:

net/ipv4/udp.c:udp_sendmsg() {
	...
	if (connected)
		rt = dst_rtable(sk_dst_check(sk, 0));
	...
}

dst_rtable() in include/net/route.h is a bare container_of_const with no
family or type check, and sk_dst_check() only discards the entry when
dst->ops->check() returns NULL.  On the IPv6 side the cookie is ignored
when sernum is set:

net/ipv6/route.c:ip6_dst_check() {
	...
	if (rt->sernum)
		return rt6_is_valid(rt) ? dst : NULL;
	...
}

and sernum is non-zero for pcpu routes derived from nexthop objects:

net/ipv6/route.c {
	...
	if (f6i->nh)
		pcpu_rt->sernum = rt_genid_ipv6(dev_net(dev));
	...
}

so sk_dst_check(sk, 0) from the IPv4 path can hand the stale rt6_info back.
The skb then gets a 20-byte IPv4 header from ip_make_skb()/__ip_make_skb()
and is handed to dst_output() == ip6_output(), where ip6_finish_output2()
reads ipv6_hdr(skb)->daddr at offsets 24..39 of a 20-byte network header.

Isn't that the same uninitialized read in ip6_finish_output2() the commit
message says this change eliminates?

The commit message states:

    the retained IPv6 dst routes the skb through ip6_output().
    ip6_finish_output2() consequently consumes the unwritten 20-byte tail.

If the retained dst is part of the described failure, should the mismatch
branch (or the code before the lookup) also invalidate the socket route
cache with sk_dst_reset(sk)?

Also, the same ip6_sk_dst_store_flow() publish happens on the !corkreq
lockless branch, which never takes the socket lock at all:

net/ipv6/udp.c:udpv6_sendmsg() {
	...
	/* Lockless fast path for the non-corking case */
	if (!corkreq) {
		...
		/* ip6_make_skb steals dst reference */
		goto out_no_dst;
	}
	...
}

Is that path covered by anything here?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
since this change adds a family recheck on the send side, the peer site in
do_ipv6_setsockopt(IPV6_ADDRFORM) looks inconsistent between TCP and UDP:

net/ipv6/ipv6_sockglue.c:do_ipv6_setsockopt() {
	...
			if (sk->sk_protocol == IPPROTO_UDP) {
				if (udp_sk(sk)->pending == AF_INET6) {
					retv = -EBUSY;
					break;
				}
			} else if (sk->sk_protocol == IPPROTO_TCP) {
				if (sk->sk_prot != &tcpv6_prot) {
					retv = -EBUSY;
					break;
				}
	...
}

The TCP branch refuses the conversion when sk_prot was swapped out (added
by commit b6f6118901d1, "ipv6: restrict IPV6_ADDRFORM operation", for the
sockmap case), while the UDP branch only looks at pending and then
overwrites the proto unconditionally:

net/ipv6/ipv6_sockglue.c:do_ipv6_setsockopt() {
	...
				WRITE_ONCE(sk->sk_prot, &udp_prot);
				WRITE_ONCE(sk->sk_socket->ops, &inet_dgram_ops);
				WRITE_ONCE(sk->sk_family, PF_INET);
	...
}

UDP sockets can have sk_prot swapped the same way.  udp_bpf_update_proto()
installs &udp_bpf_prots[family] via sock_replace_proto() and keeps the old
proto in psock->sk_proto, and sock_map_sk_state_allowed() accepts any
hashed UDP socket:

net/core/sock_map.c:sock_map_sk_state_allowed() {
	...
	if (sk_is_udp(sk))
		return sk_hashed(sk);
	...
}

A connect()ed AF_INET6 UDP socket is hashed, so sockmap membership and the
ADDRFORM preconditions can hold at the same time.  After the conversion the
sockmap-installed proto is discarded, so sock_map_close and
udp_bpf_recvmsg no longer apply.  Later, when the psock is dropped,
sk_psock_restore_proto() -> udp_bpf_update_proto(restore=true) ->
sock_replace_proto(sk, psock->sk_proto) writes udpv6_prot back onto a
socket whose sk_family is now PF_INET and whose ops are inet_dgram_ops.

Should the UDP branch also check sk->sk_prot != &udpv6_prot, the way the
TCP branch does?  Reaching this needs privileges to create and populate a
sockmap.
-- 
pw-bot: cr