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.
> >
> > Fixes: cca9bab1b72c ("tcp: use monotonic timestamps for PAWS")
> > Signed-off-by: Guillaume Nault <gnault@redhat.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
> >
> 
> 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()
> 
Indeed. I'll work on that.

> (Note the issue has been there forever on 32bit arches)
> 
Yes, I'll also update the Fixes tag.

Thanks.