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