Re: [PATCH stable 6.12] 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] Missing updates to DCCP's syn_recv_sock implementations and callers cause a build failure.

Pre-existing issues:
- [Critical] If `tcp_v4_syn_recv_sock()` hits an error path before `opt_child_init` is called, destroying the child socket will free the listener's `ipv6_pinfo` state, causing a Use-After-Free.
- [High] The exact ehash insertion race condition patched in TCP remains unfixed in DCCP for IPv4-mapped IPv6 sockets.
--

commit bed80c01fd926d5de457cacc1f99f5af86ff3590
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.

    [ ... ]

    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 c0deaafebfdc0..41e7ab58b211f 100644
> --- a/include/net/inet_connection_sock.h
> +++ b/include/net/inet_connection_sock.h
> @@ -42,7 +42,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	    sockaddr_len;
>  	int	    (*setsockopt)(struct sock *sk, int level, int optname,

[Severity: Low]
Does adding the opt_child_init parameter here require updating the DCCP
subsystem as well?

Looking at net/dccp/ipv4.c:dccp_v4_request_recv_sock() and
net/dccp/ipv6.c:dccp_v6_request_recv_sock(), their signatures still appear
to use 6 arguments, which would cause a build breakage when assigned to
icsk_af_ops->syn_recv_sock. Similarly, the caller dccp_check_req() in
net/dccp/minisocks.c appears to pass 6 arguments.

[Severity: High]
This isn't a bug introduced by this patch, but does the exact ehash
insertion race condition patched here in TCP remain unfixed in DCCP for
IPv4-mapped IPv6 sockets?

In net/dccp/ipv4.c:dccp_v4_request_recv_sock(), the child socket is inserted
into the ehash table:

	*own_req = inet_ehash_nolisten(newsk, req_to_sk(req_unhash), NULL);

But the child socket is initialized later in
net/dccp/ipv6.c:dccp_v6_request_recv_sock():

		newdp6 = (struct dccp6_sock *)newsk;
		newinet = inet_sk(newsk);
		newinet->pinet6 = &newdp6->inet6;
		newnp = inet6_sk(newsk);
		memcpy(newnp, np, sizeof(struct ipv6_pinfo));

This exposes the incompletely initialized DCCP child socket to concurrent
access while pinet6 still points to the listener.

[ ... ]

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 5afe5e57c89b5..550d64babb94e 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1796,6 +1798,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 is a pre-existing issue, but does placing opt_child_init() here leave
the error paths vulnerable to use-after-free and data corruption of the
listener socket?

If an error like a routing failure occurs earlier in tcp_v4_syn_recv_sock()
before opt_child_init() is called:

	if (!dst) {
		dst = inet_csk_route_child_sock(sk, newsk, req);
		if (!dst)
			goto put_and_exit;

The error path destroys the child socket:

put_and_exit:
	newinet->inet_opt = NULL;
	inet_csk_prepare_forced_close(newsk);
	tcp_done(newsk);
	goto exit;

Since newinet->pinet6 still points to the listener's ipv6_pinfo at this
point, would destroying the child socket free and corrupt the listener's
state? Should the opt_child_init() callback be moved earlier, immediately
after tcp_create_openreq_child()?

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