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

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch blocks only one direction of the stale-skb race (an
  IPv6 skb reaching tcp_v4_do_rcv()).
- [Medium] The commit message says that in the native-to-mapped race the
  stale IPv6 skb 'belongs to the disconnected connection, so drop it'.

--

tcp: prevent cross-family skb processing after disconnect

This commit adds tcp_v6_disconnect(), which puts back the IPv6 af_ops,
backlog_rcv and af_specific handlers after a v4-mapped socket is
disconnected. It also makes tcp_v4_do_rcv() drop skbs that are not
ETH_P_IP, and sends the MPTCP subflow teardown paths through
sk_prot->disconnect.

Link: https://lore.kernel.org/netdev/CAL4WiiqFJpmcV4=TWRpukCq6q=-729658FwGvieNcPXe97kLvQ@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: Medium]
The commit message says:

  In the reverse race, the skb belongs to the disconnected connection, so
  drop it instead of redirecting it to the IPv6 receive path.

Is the skb dropped when the socket is not owned by the user? This check
only runs when the skb goes through sk->sk_backlog_rcv. The unowned branch
of tcp_v6_rcv() calls tcp_v6_do_rcv() directly:

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);
    ...
}

tcp_v6_do_rcv() only diverts ETH_P_IP skbs:

	if (skb->protocol == htons(ETH_P_IP))
		return tcp_v4_do_rcv(sk, skb);

Take an IPv6 skb that was looked up before connect(AF_UNSPEC), where the
socket is reconnected to a v4-mapped peer before bh_lock_sock_nested().
That skb goes through tcp_v6_do_rcv() and tcp_rcv_state_process() on the
new connection, whose icsk_af_ops is now &ipv6_mapped.

In SYN_SENT, can an IPv6 SYN-ACK carrying the new ISN (old write_seq +
max_window + 2) complete the mapped connection? If so, tcp_finish_connect()
calls:

	icsk->icsk_af_ops->sk_rx_dst_set(sk, skb);

For ipv6_mapped that callback is inet_sk_rx_dst_set(), so an IPv6 rt6_info
gets cached as sk_rx_dst on the v4-mapped socket.

The __ip_options_echo() overflow is not reached on this path, because
resets go out through tcp_v6_send_reset(). I could not confirm whether the
cached IPv6 dst is later reused for IPv4 skbs. That depends on
ip6_dst_check() accepting a zero cookie.

Even so, the stale skb can still change the state of the new connection.
That contradicts the drop described in the commit message.

> 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);

[Severity: High]
After this restore, can a stale IPv4 skb be processed with IPv6 af_ops?
The new check in tcp_v4_do_rcv() only looks at skb->protocol. It does not
check whether the socket is still in v4-mapped mode.

A possible sequence:

1. An AF_INET6 socket is connected to a v4-mapped peer. tcp_v4_rcv() looks
   it up for an IPv4 segment.

2. tcp_v4_rcv() runs tcp_inbound_hash() and tcp_filter() before
   bh_lock_sock_nested(). A socket filter can widen this window, as the
   commit message describes.

3. Inside that window, connect(AF_UNSPEC) calls tcp_v6_disconnect(). That
   restores ipv6_specific, tcp_v6_do_rcv and tcp_sock_ipv6_specific. The
   socket is then connected to a native IPv6 peer.

4. The stale IPv4 skb reaches tcp_v4_do_rcv() in one of two ways. If the
   socket is not owned, tcp_v4_rcv() calls it directly. Otherwise the
   backlog drain goes through tcp_v6_do_rcv(), which forwards any ETH_P_IP
   skb without checking the socket's mode:

	if (skb->protocol == htons(ETH_P_IP))
		return tcp_v4_do_rcv(sk, skb);

5. The skb really is IPv4, so the new ETH_P_IP check lets it through.

On a SYN-ACK in SYN_SENT, tcp_finish_connect() calls
icsk_af_ops->sk_rx_dst_set(sk, skb). tcp_rcv_established() makes the same
call before any sequence check when sk_rx_dst is NULL:

	if (unlikely(!rcu_access_pointer(sk->sk_rx_dst)))
		inet_csk(sk)->icsk_af_ops->sk_rx_dst_set(sk, skb);

After the restore, that callback is inet6_sk_rx_dst_set():

	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));
	}

This stores an IPv4 rtable as sk_rx_dst and reads it as an rt6_info in
rt6_get_cookie().

Could later IPv6 segments then get this IPv4 dst attached through early
demux? ipv4_dst_check() ignores the cookie, so dst_input() could end up
calling ip_local_deliver() on IPv6 packets. I have not traced this last
step completely.

Before this patch, the socket kept ipv6_mapped ops, so an IPv4 skb still
matched them. With the restore on the normal disconnect path, an IPv4 skb
can now meet IPv6 af_ops, and nothing guards against that.

Would it be better to compare the skb family against the socket's
mapped/native mode? For example, ETH_P_IP skbs could be rejected on a
non-listening AF_INET6 socket whose icsk_af_ops != &ipv6_mapped.
Alternatively, the socket could be revalidated after bh_lock_sock_nested().

> +
> +	return ret;
> +}

[ ... ]

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