lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 5 Dec 2019 19:41:34 -0800 From: Eric Dumazet <eric.dumazet@...il.com> To: Guillaume Nault <gnault@...hat.com>, 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>, John Stultz <john.stultz@...aro.org>, Thomas Gleixner <tglx@...utronix.de> Subject: Re: [PATCH net v3 1/3] tcp: fix rejected syncookies due to stale timestamps On 12/5/19 5:49 PM, Guillaume Nault wrote: > If no synflood happens for a long enough period of time, then the > synflood timestamp isn't refreshed and jiffies can advance so much > that time_after32() can't accurately compare them any more. > > Therefore, we can end up in a situation where time_after32(now, > last_overflow + HZ) returns false, just because these two values are > too far apart. In that case, the synflood timestamp isn't updated as > it should be, which can trick tcp_synq_no_recent_overflow() into > rejecting valid syncookies. > > For example, let's consider the following scenario on a system > with HZ=1000: > > * The synflood timestamp is 0, either because that's the timestamp > of the last synflood or, more commonly, because we're working with > a freshly created socket. > > * We receive a new SYN, which triggers synflood protection. Let's say > that this happens when jiffies == 2147484649 (that is, > 'synflood timestamp' + HZ + 2^31 + 1). > > * Then tcp_synq_overflow() doesn't update the synflood timestamp, > because time_after32(2147484649, 1000) returns false. > With: > - 2147484649: the value of jiffies, aka. 'now'. > - 1000: the value of 'last_overflow' + HZ. > > * A bit later, we receive the ACK completing the 3WHS. But > cookie_v[46]_check() rejects it because tcp_synq_no_recent_overflow() > says that we're not under synflood. That's because > time_after32(2147484649, 120000) returns false. > With: > - 2147484649: the value of jiffies, aka. 'now'. > - 120000: the value of 'last_overflow' + TCP_SYNCOOKIE_VALID. > > Of course, in reality jiffies would have increased a bit, but this > condition will last for the next 119 seconds, which is far enough > to accommodate for jiffie's growth. > > Fix this by updating the overflow timestamp whenever jiffies isn't > within the [last_overflow, last_overflow + HZ] range. That shouldn't > have any performance impact since the update still happens at most once > per second. > > Now we're guaranteed to have fresh timestamps while under synflood, so > tcp_synq_no_recent_overflow() can safely use it with time_after32() in > such situations. > > Stale timestamps can still make tcp_synq_no_recent_overflow() return > the wrong verdict when not under synflood. This will be handled in the > next patch. > > For 64 bits architectures, the problem was introduced with the > conversion of ->tw_ts_recent_stamp to 32 bits integer by commit > cca9bab1b72c ("tcp: use monotonic timestamps for PAWS"). > The problem has always been there on 32 bits architectures. > > Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS") > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2") > Signed-off-by: Guillaume Nault <gnault@...hat.com> > --- > include/linux/time.h | 13 +++++++++++++ > include/net/tcp.h | 5 +++-- > 2 files changed, 16 insertions(+), 2 deletions(-) > > diff --git a/include/linux/time.h b/include/linux/time.h > index 0760a4f5a15c..30efc1f0f67d 100644 > --- a/include/linux/time.h > +++ b/include/linux/time.h > @@ -97,4 +97,17 @@ static inline bool itimerspec64_valid(const struct itimerspec64 *its) > */ > #define time_after32(a, b) ((s32)((u32)(b) - (u32)(a)) < 0) > #define time_before32(b, a) time_after32(a, b) > + > +/** > + * time_before32 - check if a 32-bit timestamp is within a given time range Wrong name ? This should be time_between32 > + * @t: the time which may be within [l,h] > + * @l: the lower bound of the range > + * @h: the higher bound of the range > + * > + * time_before32(t, l, h) returns true if @l <= @t <= @h. All operands are > + * treated as 32-bit integers. > + * > + * Equivalent to !(time_before32(@t, @l) || time_after32(@t, @h)). > + */ > +#define time_between32(t, l, h) ((u32)(h) - (u32)(l) >= (u32)(t) - (u32)(l)) > #endif > diff --git a/include/net/tcp.h b/include/net/tcp.h > index 36f195fb576a..7d734ba391fc 100644 > --- a/include/net/tcp.h > +++ b/include/net/tcp.h > @@ -494,14 +494,15 @@ 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_between32(now, last_overflow, > + 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_between32(now, last_overflow, last_overflow + HZ)) > tcp_sk(sk)->rx_opt.ts_recent_stamp = now; > } > >
Powered by blists - more mailing lists