linux-kernel - Re: [PATCH net v2] tcp: Defer ts_recent changes until req is owned

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAL+tcoDBHtRt-Ep3QORj3DnDa0Aeha5ZhQi30QP_zZkm-SrzUw@mail.gmail.com>
Date: Sat, 22 Feb 2025 23:06:18 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Wang Hai <wanghai38@...wei.com>
Cc: edumazet@...gle.com, ncardwell@...gle.com, kuniyu@...zon.com, 
	davem@...emloft.net, dsahern@...nel.org, kuba@...nel.org, pabeni@...hat.com, 
	horms@...nel.org, zhangchangzhong@...wei.com, liujian56@...wei.com, 
	yuehaibing@...wei.com, netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net v2] tcp: Defer ts_recent changes until req is owned

On Sat, Feb 22, 2025 at 6:41 PM Wang Hai <wanghai38@...wei.com> wrote:
>
> The same 5-tuple packet may be processed by different CPUSs, so two
> CPUs may receive different ack packets at the same time when the
> state is TCP_NEW_SYN_RECV.
>
> In that case, req->ts_recent in tcp_check_req may be changed concurrently,
> which will probably cause the newsk's ts_recent to be incorrectly large.
> So that tcp_validate_incoming will fail.
>
> cpu1                                    cpu2
> tcp_check_req
>                                         tcp_check_req
>  req->ts_recent = rcv_tsval = t1
>                                          req->ts_recent = rcv_tsval = t2
>
>  syn_recv_sock
>   newsk->ts_recent = req->ts_recent = t2 // t1 < t2
> tcp_child_process
>  tcp_rcv_state_process
>   tcp_validate_incoming
>    tcp_paws_check
>     if ((s32)(rx_opt->ts_recent - rx_opt->rcv_tsval) <= paws_win)
>         // t2 - t1 > paws_win, failed
>
> In tcp_check_req, Defer ts_recent changes to this skb's to fix this bug.

Honestly, from my perspective, the commit message doesn't actually
reflect what the real problem you've encountered is and what the
potential bad result could be. Your previous reply is good and
detailed, at least showing to the readers enough information to help
them revisit or analyze in the future.

>
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Wang Hai <wanghai38@...wei.com>

Otherwise, it looks good to me. Thanks!

Reviewed-by: Jason Xing <kerneljasonxing@...il.com>

> ---
> v1->v2: Modified the fix logic based on Eric's suggestion. Also modified the msg
>  net/ipv4/tcp_minisocks.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
> index b089b08e9617..53700206f498 100644
> --- a/net/ipv4/tcp_minisocks.c
> +++ b/net/ipv4/tcp_minisocks.c
> @@ -815,12 +815,6 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>
>         /* In sequence, PAWS is OK. */
>
> -       /* TODO: We probably should defer ts_recent change once
> -        * we take ownership of @req.
> -        */
> -       if (tmp_opt.saw_tstamp && !after(TCP_SKB_CB(skb)->seq, tcp_rsk(req)->rcv_nxt))
> -               WRITE_ONCE(req->ts_recent, tmp_opt.rcv_tsval);
> -
>         if (TCP_SKB_CB(skb)->seq == tcp_rsk(req)->rcv_isn) {
>                 /* Truncate SYN, it is out of window starting
>                    at tcp_rsk(req)->rcv_isn + 1. */
> @@ -869,6 +863,9 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>         if (!child)
>                 goto listen_overflow;
>
> +       if (own_req && tmp_opt.saw_tstamp && !after(TCP_SKB_CB(skb)->seq, tcp_rsk(req)->rcv_nxt))
> +               tcp_sk(child)->rx_opt.ts_recent = tmp_opt.rcv_tsval;
> +

nit: I would suggest using the following format if a re-spin is necessary:
+       if (own_req && tmp_opt.saw_tstamp &&
+           !after(TCP_SKB_CB(skb)->seq, tcp_rsk(req)->rcv_nxt))
+               tcp_sk(child)->rx_opt.ts_recent = tmp_opt.rcv_tsval;
+

Thanks,
Jason

>         if (own_req && rsk_drop_req(req)) {
>                 reqsk_queue_removed(&inet_csk(req->rsk_listener)->icsk_accept_queue, req);
>                 inet_csk_reqsk_queue_drop_and_put(req->rsk_listener, req);
> --
> 2.17.1
>