Date:   Thu, 28 Nov 2019 22:36:13 +0100
From:   Guillaume Nault <gnault@...hat.com>
To:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        Arnd Bergmann <arnd@...db.de>
Subject: [PATCH net] tcp: Avoid time_after32() underflow when handling
 syncookies

In tcp_synq_overflow() and tcp_synq_no_recent_overflow(), the
time_after32() call might underflow and return the opposite of the
expected result.

This happens after socket initialisation, when ->synq_overflow_ts and
->rx_opt.ts_recent_stamp are still set to zero. In this case, they
can't be compared reliably to the current value of jiffies using
time_after32(), because jiffies may be too far apart (especially soon
after system startup, when it's close to 2^32).

In such a situation, the erroneous time_after32() result prevents
tcp_synq_overflow() from updating ->synq_overflow_ts and
->rx_opt.ts_recent_stamp, so the problem remains until jiffies wraps
and exceeds HZ.

Practical consequences should be quite limited though, because the
time_after32() call of tcp_synq_no_recent_overflow() would also
underflow (unless jiffies wrapped since the first time_after32() call),
thus detecting a socket overflow and triggering the syncookie
verification anyway.

Also, since commit 399040847084 ("bpf: add helper to check for a valid
SYN cookie") and commit 70d66244317e ("bpf: add bpf_tcp_gen_syncookie
helper"), tcp_synq_overflow() and tcp_synq_no_recent_overflow() can be
triggered from BPF programs. Even though such programs would normally
pair these two operations, so both underflows would compensate each
other as described above, we'd better avoid exposing the problem
outside of the kernel networking stack.

Let's fix it by initialising ->rx_opt.ts_recent_stamp and
->synq_overflow_ts to a value that can be safely compared to jiffies
using time_after32(). Use "jiffies - TCP_SYNCOOKIE_VALID - 1", to
indicate that we're not in a socket overflow phase.

Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS")
Signed-off-by: Guillaume Nault <gnault@...hat.com>
---
 net/core/sock_reuseport.c | 10 ++++++++++
 net/ipv4/tcp.c            |  8 ++++++++
 2 files changed, 18 insertions(+)

diff --git a/net/core/sock_reuseport.c b/net/core/sock_reuseport.c
index f19f179538b9..87c287433a52 100644
--- a/net/core/sock_reuseport.c
+++ b/net/core/sock_reuseport.c
@@ -11,6 +11,7 @@
 #include <linux/idr.h>
 #include <linux/filter.h>
 #include <linux/rcupdate.h>
+#include <net/tcp.h>
 
 #define INIT_SOCKS 128
 
@@ -85,6 +86,15 @@ int reuseport_alloc(struct sock *sk, bool bind_inany)
 	reuse->socks[0] = sk;
 	reuse->num_socks = 1;
 	reuse->bind_inany = bind_inany;
+
+	/* synq_overflow_ts can be used for syncookies. Ensure that it has a
+	 * recent value, so that tcp_synq_overflow() and
+	 * tcp_synq_no_recent_overflow() can safely use time_after32().
+	 * Initialise it 'TCP_SYNCOOKIE_VALID + 1' jiffies in the past, to
+	 * ensure that we start in the 'no recent overflow' case.
+	 */
+	reuse->synq_overflow_ts = jiffies - TCP_SYNCOOKIE_VALID - 1;
+
 	rcu_assign_pointer(sk->sk_reuseport_cb, reuse);
 
 out:
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 9b48aec29aca..e9555db95dff 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -443,6 +443,14 @@ void tcp_init_sock(struct sock *sk)
 	tp->tsoffset = 0;
 	tp->rack.reo_wnd_steps = 1;
 
+	/* ts_recent_stamp can be used for syncookies. Ensure that it has a
+	 * recent value, so that tcp_synq_overflow() and
+	 * tcp_synq_no_recent_overflow() can safely use time_after32().
+	 * Initialise it 'TCP_SYNCOOKIE_VALID + 1' jiffies in the past, to
+	 * ensure that we start in the 'no recent overflow' case.
+	 */
+	tp->rx_opt.ts_recent_stamp = jiffies - TCP_SYNCOOKIE_VALID - 1;
+
 	sk->sk_state = TCP_CLOSE;
 
 	sk->sk_write_space = sk_stream_write_space;
-- 
2.21.0

Powered by blists - more mailing lists