Re: [PATCH net] tcp: restore IPv6 af_ops on disconnect of a v4-mapped socket

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 0 · 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 does not fully close the bug class it targets: an IPv6
  skb handed from the socket backlog to tcp_v4_do_rcv(), which then…

--

tcp: restore IPv6 af_ops on disconnect of a v4-mapped socket

This adds tcp_v6_disconnect(). When a socket that was connected to a
v4-mapped address is disconnected, it restores icsk_af_ops,
sk_backlog_rcv and af_specific to the IPv6 handlers. The connect() error
path uses the same helper, and the two MPTCP teardown paths now call
sk_prot->disconnect instead of tcp_disconnect() directly.

Closes: https://lore.kernel.org/netdev/CALf2hKvp0NPVLwr=a3+jUHtPohxbra8gox+aYAfWathJnn-J+Q@mail.gmail.com/
Link: https://lore.kernel.org/netdev/CAL4WiiqFJpmcV4=TWRpukCq6q=-729658FwGvieNcPXe97kLvQ@mail.gmail.com/
Link: https://lore.kernel.org/lkml/20261029054136.1174216-1-zhiyuzhang999@gmail.com/

> 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]
Does this also cover the opposite transition? The check only restores the
handlers when going from mapped to native. Going from a native IPv6
connection, through connect(AF_UNSPEC), to a v4-mapped connect() still
looks like it can pass an IPv6 skb from the backlog to tcp_v4_do_rcv().

In tcp_v6_rcv(), the socket lookup, tcp_v6_fill_cb() and the TCP_LISTEN
check all run before the socket spinlock is taken. Once the lock is held,
nothing checks sk_backlog_rcv again:

net/ipv6/tcp_ipv6.c:tcp_v6_rcv() {
    ...
	tcp_v6_fill_cb(skb, hdr, th);
    ...
	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);
    ...
}

Can this ordering happen?

CPU1 (IPv6 segment for the old native connection)
tcp_v6_rcv()
  __inet6_lookup_skb() finds sk
  tcp_v6_fill_cb()   /* fills header.h6 */

                     CPU0
                     connect(AF_UNSPEC)
                       tcp_v6_disconnect()
                         /* sk_backlog_rcv is tcp_v6_do_rcv, no restore */
                     connect(::ffff:a.b.c.d)
                       tcp_v6_connect()
                         sk->sk_backlog_rcv = tcp_v4_do_rcv;
                         tcp_v4_connect()   /* SYN_SENT, still owned */

  bh_lock_sock_nested()
  sock_owned_by_user() is true
  tcp_add_backlog()

                     release_sock()
                       __release_sock()
                         sk_backlog_rcv() -> tcp_v4_do_rcv()

tcp_v6_do_rcv() redirects ETH_P_IP skbs, but tcp_v4_do_rcv() does not
check skb->protocol at all.

In SYN_SENT, a stale ACK carries an ack_seq from the old connection.
tcp_rcv_synsent_state_process() rejects it, so tcp_v4_do_rcv() ends up
here:

net/ipv4/tcp_ipv4.c:tcp_v4_do_rcv() {
    ...
	reason = tcp_rcv_state_process(sk, skb);
	if (reason)
		goto reset;
	return 0;

reset:
	tcp_v4_send_reset(sk, skb, sk_rst_convert_drop_reason(reason));
    ...
}

>From there, tcp_v4_send_reset() reads ip_hdr() of an IPv6 packet. It then
passes &TCP_SKB_CB(skb)->header.h4.opt, which actually holds h6, to
ip_send_unicast_reply() -> __ip_options_echo(). That is the same 40-byte
overflow the commit message describes.

The window is narrow. Hard irqs, vCPU preemption, preemptible softirqs on
PREEMPT_RT, or a socket filter run by tcp_filter() inside the window can
make it longer.

Is an skb->protocol check also needed in the mapped receive path, in
addition to restoring the handlers on disconnect? It could drop ETH_P_IPV6
skbs, or redirect them to tcp_v6_do_rcv(), like the existing ETH_P_IP check
in tcp_v6_do_rcv().

> +
> +	return ret;
> +}
> +

[ ... ]

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