Re: [PATCH stable 5.10.y] tcp: fix potential race in tcp_v6_syn_recv_sock()

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch adds a 7th argument `opt_child_init` to the `syn_recv_sock` callback but fails to update the DCCP subsystem, leading to compilation errors.

Pre-existing issues:
- [Critical] This problem wasn't introduced by this patch, but the error path in tcp_v4_syn_recv_sock() frees the listening socket's IPv6 options via a stale pinet6 pointer.
- [High] This problem wasn't introduced by this patch, but the exact data race condition fixed for TCP IPv6-mapped sockets was left unfixed for DCCP IPv6-mapped sockets.
--

commit 9fb42b0dda20f2c1f24b669590d7c23a325895a3
Author: Eric Dumazet <edumazet@google.com>

tcp: fix potential race in tcp_v6_syn_recv_sock()
    
Code in tcp_v6_syn_recv_sock() after the call to tcp_v4_syn_recv_sock()
is done too late. After tcp_v4_syn_recv_sock(), the child socket is already
visible from TCP ehash table and other cpus might use it. Move the
problematic code in tcp_v6_mapped_child_init() and call this new helper
from tcp_v4_syn_recv_sock() before the ehash insertion.

Link: https://patch.msgid.link/20260217161205.2079883-1-edumazet@google.com

> diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h
> index cfb66f5a50766..ba99da8eedb75 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h

[ ... ]

> @@ -41,7 +41,9 @@ struct inet_connection_sock_af_ops {
>  				      struct request_sock *req,
>  				      struct dst_entry *dst,
>  				      struct request_sock *req_unhash,
> -				      bool *own_req);
> +				      bool *own_req,
> +				      void (*opt_child_init)(struct sock *newsk,
> +							     const struct sock *sk));
>  	u16	    net_header_len;
>  	u16	    net_frag_header_len;
>  	u16	    sockaddr_len;

[Severity: Low]
Does the DCCP subsystem need to be updated for this new 7th argument?

The syn_recv_sock callback signature now requires 7 arguments, but it looks
like dccp_v4_request_recv_sock() and dccp_v6_request_recv_sock() might
still be using 6 arguments, and dccp_check_req() in net/dccp/minisocks.c
might be missing the 7th argument in its call. Could this cause a build
failure when CONFIG_IP_DCCP is enabled due to an arity mismatch?

[Severity: High]
This is a pre-existing issue, but does DCCP have the same race condition for
IPv6-mapped sockets that this patch fixes for TCP?

Looking at dccp_v6_request_recv_sock() in net/dccp/ipv6.c:

dccp_v6_request_recv_sock() {
    newsk = dccp_v4_request_recv_sock(sk, skb, req, dst, ...);
    if (newsk == NULL)
        return NULL;
    
    newdp6 = (struct dccp6_sock *)newsk;
    newinet = inet_sk(newsk);
    newinet->pinet6 = &newdp6->inet6;
    newnp = inet6_sk(newsk);
    memcpy(newnp, np, sizeof(struct ipv6_pinfo));
    ...
}

It appears that dccp_v4_request_recv_sock() publishes the child socket to
the ehash table before dccp_v6_request_recv_sock() initializes its pinet6
pointer. Can a concurrent thread looking up the ehash table access the
partially initialized DCCP socket?

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 855cca214a021..f1e10db3d6253 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c

[ ... ]

> @@ -1591,6 +1593,10 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
>  	}
>  	sk_setup_caps(newsk, dst);
>  
> +#if IS_ENABLED(CONFIG_IPV6)
> +	if (opt_child_init)
> +		opt_child_init(newsk, sk);
> +#endif
>  	tcp_ca_openreq_child(newsk, dst);
>  
>  	tcp_sync_mss(newsk, dst_mtu(dst));

[Severity: Critical]
This isn't a bug introduced by this patch, but does placing the
opt_child_init() callback here leave a window for a use-after-free on the
listener's IPv6 options in the error path?

When an IPv4 SYN packet is received on an IPv6-mapped TCP listener,
tcp_create_openreq_child() bitwise-copies the listener's memory into the
new socket, meaning newinet->pinet6 points to the listener's ipv6_pinfo.

If routing fails before reaching this new callback, such as when processing
an unroutable spoofed source IP:

tcp_v4_syn_recv_sock() {
    newsk = tcp_create_openreq_child(sk, req, skb);
    ...
    if (!dst) {
        dst = inet_csk_route_child_sock(sk, newsk, req);
        if (!dst)
            goto put_and_exit;
    }
    ...
    if (opt_child_init)
        opt_child_init(newsk, sk);
    ...
put_and_exit:
    newinet->inet_opt = NULL;
    inet_csk_prepare_forced_close(newsk);
    tcp_done(newsk);
    ...
}

The error path put_and_exit will call tcp_done(newsk) and destroy the
child socket. Since opt_child_init() hasn't reassigned pinet6 yet, won't
the child socket's destructor free the listener's IPv6 options (pktoptions,
rxpmtu, opt) while the listener is still actively using them?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903000029.3508006-1-florian.fainelli@broadcom.com?part=1