netdev - Re: [PATCH v2 11/25] tcp: authopt: Implement Sequence Number Extension

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+HUmGgMAU235hMtTgucVb1GX_Ru83bngHg8-Jvy2g6BA7djsg@mail.gmail.com>
Date:   Mon, 1 Nov 2021 12:22:17 -0700
From:   Francesco Ruggeri <fruggeri@...sta.com>
To:     Leonard Crestez <cdleonard@...il.com>
Cc:     David Ahern <dsahern@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.com>,
        Priyaranjan Jha <priyarjha@...gle.com>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 11/25] tcp: authopt: Implement Sequence Number Extension

> +/* Compute SNE for a specific packet (by seq). */
> +static int compute_packet_sne(struct sock *sk, struct tcp_authopt_info *info,
> +                             u32 seq, bool input, __be32 *sne)
> +{
> +       u32 rcv_nxt, snd_nxt;
> +
> +       // We can't use normal SNE computation before reaching TCP_ESTABLISHED
> +       // For TCP_SYN_SENT the dst_isn field is initialized only after we
> +       // validate the remote SYN/ACK
> +       // For TCP_NEW_SYN_RECV there is no tcp_authopt_info at all
> +       if (sk->sk_state == TCP_SYN_SENT ||
> +           sk->sk_state == TCP_NEW_SYN_RECV ||
> +           sk->sk_state == TCP_LISTEN)
> +               return 0;
> +

In case of TCP_NEW_SYN_RECV, if our SYNACK had sequence number
0xffffffff, we will receive an ACK sequence number of 0, which
should have sne = 1.

In a somewhat similar corner case, when we receive a SYNACK to
our SYN in tcp_rcv_synsent_state_process, if the SYNACK has
sequence number 0xffffffff, we set tp->rcv_nxt to 0, and we
should set sne to 1.

There may be more similar corner cases related to a wraparound
during the handshake.

Since as you pointed out all we need is "recent" valid <sne, seq>
pairs as reference, rather than relying on rcv_sne being paired
with tp->rcv_nxt (and similarly for snd_sne and tp->snd_nxt),
would it be easier to maintain reference <sne, seq> pairs for send
and receive in tcp_authopt_info, appropriately handle the different
handshake cases and initialize the pairs, and only then track them
in tcp_rcv_nxt_update and tcp_rcv_snd_update?

>  static void tcp_rcv_nxt_update(struct tcp_sock *tp, u32 seq)
>  {
>         u32 delta = seq - tp->rcv_nxt;
>
>         sock_owned_by_me((struct sock *)tp);
> +       tcp_authopt_update_rcv_sne(tp, seq);
>         tp->bytes_received += delta;
>         WRITE_ONCE(tp->rcv_nxt, seq);
>  }
>

Since rcv_sne and tp->rcv_nxt are not updated atomically, could
there ever be a case where a reader might use the new sne with
the old rcv_nxt?

Francesco