Re: [PATCH net v3] tcp: prevent cross-family skb processing after disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] tcp: stale IPv6 skb processed on v4-mapped socket in tcp_v6_do_rcv()
- [Critical] tcp: rtable/rt6_info type confusion in tcp_v4_do_rcv() after reconnect
--

commit b8863396ea09db4835f451ceb38563e379ce7144
Author: Eric Dumazet <edumazet@kernel.org>

tcp: prevent cross-family skb processing after disconnect

This commit adds tcp_v6_disconnect() to restore IPv6 socket handlers when
disconnecting a v4-mapped IPv6 socket, routes MPTCP subflow disconnects
through sk_prot->disconnect, and drops non-IPv4 packets in tcp_v4_do_rcv().

Link: https://lore.kernel.org/netdev/CALf2hKvp0NPVLwr=a3+jUHtPohxbra8gox+aYAfWathJnn-J+Q@mail.gmail.com/

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index a7f9242f1e429..8fa6564d2b602 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1836,6 +1836,11 @@ int tcp_v4_do_rcv(struct sock *sk, struct sk_buff *skb)
>  {
>  	enum skb_drop_reason reason;
>  
> +	if (unlikely(skb->protocol != htons(ETH_P_IP))) {
> +		reason = SKB_DROP_REASON_INVALID_PROTO;
> +		goto err_discard;
> +	}
> +

[Severity: Critical]
Does this check in tcp_v4_do_rcv() still miss the non-backlogged path when a
socket reconnects from native IPv6 to a v4-mapped address?

In tcp_v6_rcv(), if the user thread completes connect(AF_UNSPEC), reconnects
to a v4-mapped address, and finishes release_sock(sk) before tcp_v6_rcv()
acquires bh_lock_sock_nested(sk), sock_owned_by_user(sk) will be false:

net/ipv6/tcp_ipv6.c:tcp_v6_rcv() {
	...
	bh_lock_sock_nested(sk);
	tcp_segs_in(tcp_sk(sk), skb);
	ret = 0;
	if (!sock_owned_by_user(sk)) {
		ret = tcp_v6_do_rcv(sk, skb);
	} else {
		drop_reason = tcp_add_backlog(sk, skb);
	...
}

Instead of going through sk->sk_backlog_rcv (tcp_v4_do_rcv), tcp_v6_rcv()
calls tcp_v6_do_rcv(sk, skb) directly. Because tcp_v6_do_rcv() only redirects
ETH_P_IP packets and does not verify whether sk was switched to v4-mapped mode
(sk->sk_backlog_rcv == tcp_v4_do_rcv), it processes the stale ETH_P_IPV6 skb
while icsk->icsk_af_ops is ipv6_mapped.

When sk is in TCP_ESTABLISHED (or reaches tcp_finish_connect() in
TCP_SYN_SENT), tcp_rcv_established() then runs before sequence validation:

net/ipv4/tcp_input.c:tcp_rcv_established() {
	...
	if (unlikely(!rcu_access_pointer(sk->sk_rx_dst)))
		inet_csk(sk)->icsk_af_ops->sk_rx_dst_set(sk, skb);
	...
}

This invokes inet_sk_rx_dst_set() on the IPv6 skb and caches its rt6_info in
sk->sk_rx_dst of the v4-mapped socket, which tcp_v4_early_demux() can then
attach to subsequent IPv4 packets. Should tcp_v6_do_rcv() also reject
non-ETH_P_IP packets when sk->sk_backlog_rcv == tcp_v4_do_rcv?

> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index e006a30360eae..a93410aab66e6 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
[ ... ]
> @@ -2093,6 +2099,19 @@ static int tcp_v6_init_sock(struct sock *sk)
>  	return 0;
>  }
>  
> +static int tcp_v6_disconnect(struct sock *sk, int flags)
> +{
> +	int ret = tcp_disconnect(sk, flags);
> +
> +	/* A socket used for a v4-mapped connection must not keep IPv4
> +	 * handlers when it is reused for listen() or a native IPv6 connect().
> +	 */
> +	if (sk->sk_backlog_rcv == tcp_v4_do_rcv)
> +		tcp_v6_restore_af_ops(sk);
> +
> +	return ret;
> +}

[Severity: Critical]
With tcp_v6_disconnect() restoring icsk->icsk_af_ops to ipv6_specific and
sk->sk_backlog_rcv to tcp_v6_do_rcv, can a racing IPv4 packet cause
rtable/rt6_info type confusion when the socket is reconnected to a native IPv6
peer?

If tcp_v4_rcv() looks up a v4-mapped AF_INET6 socket via __inet_lookup_skb()
and the socket is disconnected and reconnected to a native IPv6 address before
bh_lock_sock_nested(sk), the in-flight ETH_P_IP skb still reaches
tcp_v4_do_rcv(sk, skb) either directly from tcp_v4_rcv() when
!sock_owned_by_user(sk), or from release_sock() via tcp_v6_do_rcv():

net/ipv6/tcp_ipv6.c:tcp_v6_do_rcv() {
	...
	if (skb->protocol == htons(ETH_P_IP))
		return tcp_v4_do_rcv(sk, skb);
	...
}

In tcp_v4_do_rcv(), skb->protocol != htons(ETH_P_IP) passes because the packet
is IPv4, while sk is now a native IPv6 socket with icsk->icsk_af_ops set to
ipv6_specific. When tcp_rcv_established() (or tcp_finish_connect()) calls
icsk->icsk_af_ops->sk_rx_dst_set(sk, skb), it invokes inet6_sk_rx_dst_set() on
an IPv4 rtable:

net/ipv6/tcp_ipv6.c:inet6_sk_rx_dst_set() {
	struct dst_entry *dst = skb_dst(skb);

	if (dst && dst_hold_safe(dst)) {
		rcu_assign_pointer(sk->sk_rx_dst, dst);
		sk->sk_rx_dst_ifindex = skb->skb_iif;
		sk->sk_rx_dst_cookie = rt6_get_cookie(dst_rt6_info(dst));
	}
}

Can this cast the IPv4 rtable to rt6_info in rt6_get_cookie() (dereferencing
rt->from if the overlapping sernum bytes are zero) and cache an IPv4 rtable in
sk->sk_rx_dst of the native IPv6 socket?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103103633.2707416-1-zhiyuzhang999@gmail.com?part=1