From: Eric Dumazet <edumazet@google.com>
To: Guillaume Nault <gnault@redhat.com>
Cc: David Miller <davem@davemloft.net>,
Jakub Kicinski <jakub.kicinski@netronome.com>,
netdev <netdev@vger.kernel.org>, Arnd Bergmann <arnd@arndb.de>
Subject: Re: [PATCH net] tcp: Avoid time_after32() underflow when handling syncookies
Date: Thu, 28 Nov 2019 14:04:19 -0800 [thread overview]
Message-ID: <CANn89i+G0jCU=JtSit3X9w+SaExgbbo-d1x4UEkTEJRdypN3gQ@mail.gmail.com> (raw)
In-Reply-To: <2601e43617d707a28f60f2fe6927b1aaaa0a37f8.1574976866.git.gnault@redhat.com>
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()
(Note the issue has been there forever on 32bit arches)
next prev parent reply other threads:[~2019-11-28 22:04 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-28 21:36 [PATCH net] tcp: Avoid time_after32() underflow when handling syncookies Guillaume Nault
2019-11-28 22:04 ` Eric Dumazet [this message]
2019-11-29 8:13 ` Guillaume Nault
2019-12-02 21:51 ` Guillaume Nault
2019-12-02 22:23 ` Eric Dumazet
2019-12-04 0:46 ` Guillaume Nault
2019-12-04 2:20 ` Eric Dumazet
2019-12-04 14:34 ` Guillaume Nault
2019-12-04 16:53 ` Eric Dumazet
2019-12-04 18:05 ` Guillaume Nault
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANn89i+G0jCU=JtSit3X9w+SaExgbbo-d1x4UEkTEJRdypN3gQ@mail.gmail.com' \
--to=edumazet@google.com \
--cc=arnd@arndb.de \
--cc=davem@davemloft.net \
--cc=gnault@redhat.com \
--cc=jakub.kicinski@netronome.com \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).