[PATCH net] tcp: fix race in tcp_v6_syn_recv_sock()

[PATCH net] tcp: fix race in tcp_v6_syn_recv_sock()

[Eric Dumazet]

tcp_v6_syn_recv_sock() calls ip6_dst_store() before
inet_sk(newsk)->pinet6 has been set up.

This means ip6_dst_store() writes over the parent (listener)
np->dst_cookie.

This is racy because multiple threads could share the same
parent and their final np->dst_cookie could be wrong.

Move ip6_dst_store() call after inet_sk(newsk)->pinet6
has been changed and after the copy of parent ipv6_pinfo.

Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/ipv6/tcp_ipv6.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 8c577b651bfcd2f94b45e339ed4a2b47e93ff17a..729faf8bd366ad25d093a4ae931fb46ebd45b79c 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1439,7 +1439,6 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
 	 */
 
 	newsk->sk_gso_type = SKB_GSO_TCPV6;
-	ip6_dst_store(newsk, dst, NULL, NULL);
 	inet6_sk_rx_dst_set(newsk, skb);
 
 	inet_sk(newsk)->pinet6 = tcp_inet6_sk(newsk);
@@ -1450,6 +1449,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
 
 	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
 
+	ip6_dst_store(newsk, dst, NULL, NULL);
+
 	newsk->sk_v6_daddr = ireq->ir_v6_rmt_addr;
 	newnp->saddr = ireq->ir_v6_loc_addr;
 	newsk->sk_v6_rcv_saddr = ireq->ir_v6_loc_addr;
-- 
2.45.1.467.gbab1589fc0-goog

Re: [PATCH net] tcp: fix race in tcp_v6_syn_recv_sock()

[Simon Horman]

+ David Ahern

On Thu, Jun 06, 2024 at 03:46:51PM +0000, Eric Dumazet wrote:
> tcp_v6_syn_recv_sock() calls ip6_dst_store() before
> inet_sk(newsk)->pinet6 has been set up.
> 
> This means ip6_dst_store() writes over the parent (listener)
> np->dst_cookie.
> 
> This is racy because multiple threads could share the same
> parent and their final np->dst_cookie could be wrong.
> 
> Move ip6_dst_store() call after inet_sk(newsk)->pinet6
> has been changed and after the copy of parent ipv6_pinfo.
> 
> Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Reviewed-by: Simon Horman <horms@kernel.org>

> ---
>  net/ipv6/tcp_ipv6.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index 8c577b651bfcd2f94b45e339ed4a2b47e93ff17a..729faf8bd366ad25d093a4ae931fb46ebd45b79c 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -1439,7 +1439,6 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
>  	 */
>  
>  	newsk->sk_gso_type = SKB_GSO_TCPV6;
> -	ip6_dst_store(newsk, dst, NULL, NULL);
>  	inet6_sk_rx_dst_set(newsk, skb);
>  
>  	inet_sk(newsk)->pinet6 = tcp_inet6_sk(newsk);
> @@ -1450,6 +1449,8 @@ static struct sock *tcp_v6_syn_recv_sock(const struct sock *sk, struct sk_buff *
>  
>  	memcpy(newnp, np, sizeof(struct ipv6_pinfo));
>  
> +	ip6_dst_store(newsk, dst, NULL, NULL);
> +
>  	newsk->sk_v6_daddr = ireq->ir_v6_rmt_addr;
>  	newnp->saddr = ireq->ir_v6_loc_addr;
>  	newsk->sk_v6_rcv_saddr = ireq->ir_v6_loc_addr;
> -- 
> 2.45.1.467.gbab1589fc0-goog
> 
> 

Re: [PATCH net] tcp: fix race in tcp_v6_syn_recv_sock()

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu,  6 Jun 2024 15:46:51 +0000 you wrote:
> tcp_v6_syn_recv_sock() calls ip6_dst_store() before
> inet_sk(newsk)->pinet6 has been set up.
> 
> This means ip6_dst_store() writes over the parent (listener)
> np->dst_cookie.
> 
> This is racy because multiple threads could share the same
> parent and their final np->dst_cookie could be wrong.
> 
> [...]

Here is the summary with links:
  - [net] tcp: fix race in tcp_v6_syn_recv_sock()
    https://git.kernel.org/netdev/net/c/d37fe4255abe

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net] tcp: fix race in tcp_v6_syn_recv_sock()

[David Ahern]

On 6/8/24 7:25 AM, Simon Horman wrote:
> + David Ahern
> 
> On Thu, Jun 06, 2024 at 03:46:51PM +0000, Eric Dumazet wrote:
>> tcp_v6_syn_recv_sock() calls ip6_dst_store() before
>> inet_sk(newsk)->pinet6 has been set up.
>>
>> This means ip6_dst_store() writes over the parent (listener)
>> np->dst_cookie.
>>
>> This is racy because multiple threads could share the same
>> parent and their final np->dst_cookie could be wrong.
>>
>> Move ip6_dst_store() call after inet_sk(newsk)->pinet6
>> has been changed and after the copy of parent ipv6_pinfo.
>>
>> Fixes: e994b2f0fb92 ("tcp: do not lock listener to process SYN packets")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
> 
> Reviewed-by: Simon Horman <horms@kernel.org>
> 
>> ---
>>  net/ipv6/tcp_ipv6.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>

Reviewed-by: David Ahern <dsahern@kernel.org>