[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <559ac473-b771-4936-8afc-62ae39ec540b@arista.com>
Date: Thu, 2 Nov 2023 21:28:49 +0000
From: Dmitry Safonov <dima@...sta.com>
To: Kuniyuki Iwashima <kuniyu@...zon.com>
Cc: Salam Noureddine <noureddine@...sta.com>,
Kuniyuki Iwashima <kuni1840@...il.com>, netdev@...r.kernel.org,
Francesco Ruggeri <fruggeri05@...il.com>,
"David S. Miller" <davem@...emloft.net>, Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>, Paolo Abeni <pabeni@...hat.com>,
David Ahern <dsahern@...nel.org>
Subject: Re: [PATCH v1 net] tcp: Fix SYN option room calculation for TCP-AO.
On 11/2/23 21:05, Kuniyuki Iwashima wrote:
> When building SYN packet in tcp_syn_options(), MSS, TS, WS, and
> SACKPERM are used without checking the remaining bytes in the
> options area.
>
> To keep that logic as is, we limit the TCP-AO MAC length in
> tcp_ao_parse_crypto(). Currently, the limit is calculated as below.
>
> MAX_TCP_OPTION_SPACE - TCPOLEN_TSTAMP_ALIGNED
> - TCPOLEN_WSCALE_ALIGNED
> - TCPOLEN_SACKPERM_ALIGNED
>
> This looks confusing as (1) we pack SACKPERM into the leading
> 2-bytes of the aligned 12-bytes of TS and (2) TCPOLEN_MSS_ALIGNED
> is not used. Fortunately, the calculated limit is not wrong as
> TCPOLEN_SACKPERM_ALIGNED and TCPOLEN_MSS_ALIGNED are the same value.
>
> However, we should use the proper constant in the formula.
>
> MAX_TCP_OPTION_SPACE - TCPOLEN_MSS_ALIGNED
> - TCPOLEN_TSTAMP_ALIGNED
> - TCPOLEN_WSCALE_ALIGNED
>
> Fixes: 4954f17ddefc ("net/tcp: Introduce TCP_AO setsockopt()s")
> Signed-off-by: Kuniyuki Iwashima <kuniyu@...zon.com>
Sharp eye!
Thanks for the patch and adjusting the comment.
Reviewed-by: Dmitry Safonov <dima@...sta.com>
> ---
> net/ipv4/tcp_ao.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index ef5472ed6158..7696417d0640 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -1315,7 +1315,8 @@ static int tcp_ao_parse_crypto(struct tcp_ao_add *cmd, struct tcp_ao_key *key)
> key->maclen = cmd->maclen ?: 12; /* 12 is the default in RFC5925 */
>
> /* Check: maclen + tcp-ao header <= (MAX_TCP_OPTION_SPACE - mss
> - * - tstamp - wscale - sackperm),
> + * - tstamp (including sackperm)
> + * - wscale),
> * see tcp_syn_options(), tcp_synack_options(), commit 33ad798c924b.
> *
> * In order to allow D-SACK with TCP-AO, the header size should be:
> @@ -1342,9 +1343,9 @@ static int tcp_ao_parse_crypto(struct tcp_ao_add *cmd, struct tcp_ao_key *key)
> * large to leave sufficient option space.
> */
> syn_tcp_option_space = MAX_TCP_OPTION_SPACE;
> + syn_tcp_option_space -= TCPOLEN_MSS_ALIGNED;
> syn_tcp_option_space -= TCPOLEN_TSTAMP_ALIGNED;
> syn_tcp_option_space -= TCPOLEN_WSCALE_ALIGNED;
> - syn_tcp_option_space -= TCPOLEN_SACKPERM_ALIGNED;
> if (tcp_ao_len(key) > syn_tcp_option_space) {
> err = -EMSGSIZE;
> goto err_kfree;
Thanks,
Dmitry
Powered by blists - more mailing lists