Re: [PATCH net] tcp: Avoid time_after32() underflow when handling syncookies

[Guillaume Nault <gnault@redhat.com>]

On Thu, Nov 28, 2019 at 02:04:19PM -0800, Eric Dumazet wrote:
> On Thu, Nov 28, 2019 at 1:36 PM Guillaume Nault <gnault@redhat.com> wrote:
> >
> > 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.
> >
> 
> A listener could be live for one year, and flip its ' I am under
> synflood' status every 24 days (assuming HZ=1000)
> 
> You only made sure the first 24 days are ok, but the problem is still there.
> 
> We need to refresh the values, maybe in tcp_synq_no_recent_overflow()
>
Yes, but can't we refresh it in tcp_synq_overflow() instead? We
basically always want to update the timestamp, unless it's already in
the [last_overflow, last_overflow + HZ] interval:

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 36f195fb576a..1a3d76dafba8 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -494,14 +494,16 @@ static inline void tcp_synq_overflow(const struct sock *sk)
 		reuse = rcu_dereference(sk->sk_reuseport_cb);
 		if (likely(reuse)) {
 			last_overflow = READ_ONCE(reuse->synq_overflow_ts);
-			if (time_after32(now, last_overflow + HZ))
+			if (time_before32(now, last_overflow) ||
+			    time_after32(now, last_overflow + HZ))
 				WRITE_ONCE(reuse->synq_overflow_ts, now);
 			return;
 		}
 	}
 
 	last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
-	if (time_after32(now, last_overflow + HZ))
+	if (time_before32(now, last_overflow) ||
+	    time_after32(now, last_overflow + HZ))
 		tcp_sk(sk)->rx_opt.ts_recent_stamp = now;
 }
 
This way, tcp_synq_no_recent_overflow() should always have a recent
timestamp to work on, unless tcp_synq_overflow() wasn't called. But I
can't see this case happening for a legitimate connection (unless I've
missed something of course).

One could send an ACK without a SYN and get into this situation, but
then the timestamp value doesn't have too much importance since we have
to drop the connection anyway. So, even though an expired timestamp
could let the packet pass the tcp_synq_no_recent_overflow() test, the
syncookie validation would fail. So the packet is correctly rejected in
any case.