Re: [PATCH 05/12] ipv6: xfrm: use full sockets in local error paths

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ipv6: xfrm: use full sockets in local error paths

xfrm6_local_rxpmtu() and xfrm6_local_error() used skb->sk directly and cast
it to a full inet/IPv6 socket, which is wrong when a SYN-ACK skb is owned by
a TCP_NEW_SYN_RECV request_sock.  Both callbacks now resolve the owner with
skb_to_full_sk() and return early when there is no full socket attached.

> diff --git a/net/ipv6/xfrm6_output.c b/net/ipv6/xfrm6_output.c
> index 512bdaf136997..44b221a09a0c8 100644
> --- a/net/ipv6/xfrm6_output.c
> +++ b/net/ipv6/xfrm6_output.c

[ ... ]

> @@ -31,7 +34,10 @@ void xfrm6_local_error(struct sk_buff *skb, u32 mtu)
>  {
>  	struct flowi6 fl6;
>  	const struct ipv6hdr *hdr;
> -	struct sock *sk = skb->sk;
> +	struct sock *sk = skb_to_full_sk(skb);
> +
> +	if (!sk)
> +		return;

[Severity: High]
Should the IPv4 sibling of this same afinfo->local_error hook get the same
treatment?  xfrm4_local_error() in net/ipv4/xfrm4_output.c still hands the
raw skb->sk to inet_sk():

net/ipv4/xfrm4_output.c:xfrm4_local_error() {
	hdr = skb->encapsulation ? inner_ip_hdr(skb) : ip_hdr(skb);
	ip_local_error(skb->sk, EMSGSIZE, hdr->daddr,
		       inet_sk(skb->sk)->inet_dport, mtu);
}

inet_dport is skc_dport and stays in bounds, but ip_local_error() starts
with:

net/ipv4/ip_sockglue.c:ip_local_error() {
	if (!inet_test_bit(RECVERR, sk))
		return;

which expands to test_bit(INET_FLAGS_RECVERR, &inet_sk(sk)->inet_flags).
inet_flags sits after the embedded struct sock in struct inet_sock, well
past the end of a tcp_request_sock allocation, so isn't this the same read
beyond the request_sock allocation that the commit message describes for
IPv6?  And if that out-of-bounds bit happens to read as set, execution
continues into sock_queue_err_skb(sk, skb), which touches
sk->sk_error_queue - also outside the request_sock object.

Is that path reachable today?  For a SYN-ACK, tcp_make_synack() attaches the
skb to the request sock:

net/ipv4/tcp_output.c:tcp_make_synack() {
	case TCP_SYNACK_NORMAL:
	case TCP_SYNACK_RETRANS:
		skb_set_owner_edemux(skb, req_to_sk(req));

and for an IPTFS SA the output path goes
iptfs_output_collect()->iptfs_is_too_big(), which gates on the sk argument
(the listener) rather than on skb->sk:

net/xfrm/xfrm_iptfs.c:iptfs_is_too_big() {
	if (sk)
		xfrm_local_error(skb, pmtu);

and the AF_INET branch of the dispatcher does no full-socket check either:

net/xfrm/xfrm_output.c:xfrm_local_error() {
	if (skb->protocol == htons(ETH_P_IP))
		proto = AF_INET;

The classic ESP path avoids this because xfrm4_tunnel_check_size() checks
skb->sk && sk_fullsock(skb->sk) before calling xfrm_local_error(), so the
contract looks like it is enforced only at some call sites.  Would it make
sense to fix xfrm4_local_error() in the same series, or push the
skb_to_full_sk() check up into xfrm_local_error() so both families are
covered?

>  
>  	hdr = skb->encapsulation ? inner_ipv6_hdr(skb) : ipv6_hdr(skb);
>  	fl6.fl6_dport = inet_sk(sk)->inet_dport;

[Severity: Low]
Does this now report the wrong offender port?  sk_to_full_sk() replaces the
request sock with the listener:

include/net/inet_sock.h:sk_to_full_sk() {
	if (sk && READ_ONCE(sk->sk_state) == TCP_NEW_SYN_RECV)
		sk = inet_reqsk(sk)->rsk_listener;

A listening socket has inet_dport == 0, while fl6.daddr on the next line
still comes from the packet, i.e. the real peer.

ipv6_local_error() stores that value:

net/ipv6/datagram.c:ipv6_local_error() {
	serr->port = fl6->fl6_dport;

and ipv6_recv_error() hands it to userspace:

net/ipv6/datagram.c:ipv6_recv_error() {
		sin->sin6_port = serr->port;

So with IPV6_RECVERR set on the listener, the SO_EE_OFFENDER sockaddr_in6
ends up as <real peer address, port 0>.  Before this change the same
expression read the request sock's skc_dport, which is the request's actual
remote port.  Would using the request's remote port (ir_rmt_port) when the
skb owner is a request sock keep both halves of the error record
consistent?