2023-10-07 09:28:59

by George Guo

[permalink] [raw]
Subject: [PATCH] tcp: fix secure_{tcp, tcpv6}_ts_off call parameter order mistake

From: George Guo <[email protected]>

Fix secure_tcp_ts_off and secure_tcpv6_ts_off call parameter order mistake

Signed-off-by: George Guo <[email protected]>
---
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 = 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 -= tsoff;
}

diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 27140e5cdc06..3d6c9b286b5a 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_buff *skb)
{
- return secure_tcp_ts_off(net, ip_hdr(skb)->daddr, ip_hdr(skb)->saddr);
+ return secure_tcp_ts_off(net, ip_hdr(skb)->saddr, ip_hdr(skb)->daddr);
}

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 = secure_tcpv6_ts_off(sock_net(sk),
- ipv6_hdr(skb)->daddr.s6_addr32,
- ipv6_hdr(skb)->saddr.s6_addr32);
+ ipv6_hdr(skb)->saddr.s6_addr32,
+ ipv6_hdr(skb)->daddr.s6_addr32);
tcp_opt.rcv_tsecr -= tsoff;
}

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 3a88545a265d..ce9cc4c43cf2 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_buff *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


2023-10-07 10:51:07

by Florian Westphal

[permalink] [raw]
Subject: Re: [PATCH] tcp: fix secure_{tcp, tcpv6}_ts_off call parameter order mistake

George Guo <[email protected]> wrote:
> From: George Guo <[email protected]>
>
> Fix secure_tcp_ts_off and secure_tcpv6_ts_off call parameter order mistake

Could you please send a v2, targetting net-next, that clearly
says that this is a cleanup?

[ It doesn't matter if we pass "saddr, daddr" or
"daddr, saddr" as long as both "init" and "check"
functions agree on the ordering ]

2023-10-09 02:10:10

by George Guo

[permalink] [raw]
Subject: [PATCH v2] tcp: cleanup secure_{tcp, tcpv6}_ts_off

Correct secure_tcp_ts_off and secure_tcpv6_ts_off call parameter order

Signed-off-by: George Guo <[email protected]>
---
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 = 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 -= 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_buff *skb)
{
- return secure_tcp_ts_off(net, ip_hdr(skb)->daddr, ip_hdr(skb)->saddr);
+ return secure_tcp_ts_off(net, ip_hdr(skb)->saddr, ip_hdr(skb)->daddr);
}

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 = secure_tcpv6_ts_off(sock_net(sk),
- ipv6_hdr(skb)->daddr.s6_addr32,
- ipv6_hdr(skb)->saddr.s6_addr32);
+ ipv6_hdr(skb)->saddr.s6_addr32,
+ ipv6_hdr(skb)->daddr.s6_addr32);
tcp_opt.rcv_tsecr -= 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_buff *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

2023-10-09 07:19:30

by Eric Dumazet

[permalink] [raw]
Subject: Re: [PATCH v2] tcp: cleanup secure_{tcp, tcpv6}_ts_off

On Mon, Oct 9, 2023 at 5:12 AM George Guo <[email protected]> wrote:
>
> 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 <[email protected]>
> ---
> 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 = 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 -= 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_buff *skb)
> {
> - return secure_tcp_ts_off(net, ip_hdr(skb)->daddr, ip_hdr(skb)->saddr);
> + return secure_tcp_ts_off(net, ip_hdr(skb)->saddr, ip_hdr(skb)->daddr);
> }
>
> 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 = secure_tcpv6_ts_off(sock_net(sk),
> - ipv6_hdr(skb)->daddr.s6_addr32,
> - ipv6_hdr(skb)->saddr.s6_addr32);
> + ipv6_hdr(skb)->saddr.s6_addr32,
> + ipv6_hdr(skb)->daddr.s6_addr32);
> tcp_opt.rcv_tsecr -= 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_buff *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
>