Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:5 as permitted sender) client-ip=2620:137:e000::3:5;
MIME-Version: 1.0
References: <20231007105019.GA20662@breakpoint.cc> <20231009021108.3203928-1-guodongtai@kylinos.cn>
In-Reply-To: <20231009021108.3203928-1-guodongtai@kylinos.cn>
From:   Eric Dumazet <edumazet@google.com>
Date:   Mon, 9 Oct 2023 09:18:58 +0200
Message-ID: <CANn89i+OL_kPwmHEW-KBGdd-prAGC3NaQG4MNq4ZEvWpw7Q-7A@mail.gmail.com>
Subject: Re: [PATCH v2] tcp: cleanup secure_{tcp, tcpv6}_ts_off
To:     George Guo <guodongtai@kylinos.cn>
Cc:     fw@strlen.de, davem@davemloft.net, dongtai.guo@linux.dev,
        dsahern@kernel.org, kuba@kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, pabeni@redhat.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 9, 2023 at 5:12=E2=80=AFAM George Guo <guodongtai@kylinos.cn> w=
rote:
>
> Correct secure_tcp_ts_off and secure_tcpv6_ts_off call parameter order
>

I do not think this patch is correct.

We have to exchange saddr/daddr from an incoming packet in order to compute
a hash if the function expects saddr to be the local host address, and
daddr being the remote peer address.

For instance, tcp_v4_connect() uses :

WRITE_ONCE(tp->tsoffset,
                          secure_tcp_ts_off(net, inet->inet_saddr,
                                                        inet->inet_daddr));

While when receiving a packet from the other peer, it correctly swaps
saddr/daddr

tcp_v4_init_ts_off(const struct net *net, const struct sk_buff *skb)
{
 return secure_tcp_ts_off(net, ip_hdr(skb)->daddr, ip_hdr(skb)->saddr);
}


> Signed-off-by: George Guo <guodongtai@kylinos.cn>
> ---
>  net/ipv4/syncookies.c | 4 ++--
>  net/ipv4/tcp_ipv4.c   | 2 +-
>  net/ipv6/syncookies.c | 4 ++--
>  net/ipv6/tcp_ipv6.c   | 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/net/ipv4/syncookies.c b/net/ipv4/syncookies.c
> index dc478a0574cb..537f368a0b66 100644
> --- a/net/ipv4/syncookies.c
> +++ b/net/ipv4/syncookies.c
> @@ -360,8 +360,8 @@ struct sock *cookie_v4_check(struct sock *sk, struct =
sk_buff *skb)
>
>         if (tcp_opt.saw_tstamp && tcp_opt.rcv_tsecr) {
>                 tsoff =3D secure_tcp_ts_off(sock_net(sk),
> -                                         ip_hdr(skb)->daddr,
> -                                         ip_hdr(skb)->saddr);
> +                                         ip_hdr(skb)->saddr,
> +                                         ip_hdr(skb)->daddr);
>                 tcp_opt.rcv_tsecr -=3D tsoff;
>         }
>
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index a441740616d7..54717d261693 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -104,7 +104,7 @@ static u32 tcp_v4_init_seq(const struct sk_buff *skb)
>
>  static u32 tcp_v4_init_ts_off(const struct net *net, const struct sk_buf=
f *skb)
>  {
> -       return secure_tcp_ts_off(net, ip_hdr(skb)->daddr, ip_hdr(skb)->sa=
ddr);
> +       return secure_tcp_ts_off(net, ip_hdr(skb)->saddr, ip_hdr(skb)->da=
ddr);
>  }
>
>  int tcp_twsk_unique(struct sock *sk, struct sock *sktw, void *twp)
> diff --git a/net/ipv6/syncookies.c b/net/ipv6/syncookies.c
> index 5014aa663452..9af484a4d518 100644
> --- a/net/ipv6/syncookies.c
> +++ b/net/ipv6/syncookies.c
> @@ -162,8 +162,8 @@ struct sock *cookie_v6_check(struct sock *sk, struct =
sk_buff *skb)
>
>         if (tcp_opt.saw_tstamp && tcp_opt.rcv_tsecr) {
>                 tsoff =3D secure_tcpv6_ts_off(sock_net(sk),
> -                                           ipv6_hdr(skb)->daddr.s6_addr3=
2,
> -                                           ipv6_hdr(skb)->saddr.s6_addr3=
2);
> +                                           ipv6_hdr(skb)->saddr.s6_addr3=
2,
> +                                           ipv6_hdr(skb)->daddr.s6_addr3=
2);
>                 tcp_opt.rcv_tsecr -=3D tsoff;
>         }
>
> diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
> index bfe7d19ff4fd..7e2f924725c6 100644
> --- a/net/ipv6/tcp_ipv6.c
> +++ b/net/ipv6/tcp_ipv6.c
> @@ -119,8 +119,8 @@ static u32 tcp_v6_init_seq(const struct sk_buff *skb)
>
>  static u32 tcp_v6_init_ts_off(const struct net *net, const struct sk_buf=
f *skb)
>  {
> -       return secure_tcpv6_ts_off(net, ipv6_hdr(skb)->daddr.s6_addr32,
> -                                  ipv6_hdr(skb)->saddr.s6_addr32);
> +       return secure_tcpv6_ts_off(net, ipv6_hdr(skb)->saddr.s6_addr32,
> +                                  ipv6_hdr(skb)->daddr.s6_addr32);
>  }
>
>  static int tcp_v6_pre_connect(struct sock *sk, struct sockaddr *uaddr,
> --
> 2.34.1
>