netdev - Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a child socket

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANn89i+RHVmA2Mc8x0NdHZjWsw4UtgZ5ymbWBBxLgv_YczUjvg@mail.gmail.com>
Date:   Thu, 5 Dec 2019 10:14:15 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Guillaume Nault <gnault@...hat.com>
Cc:     Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net v2 2/2] tcp: tighten acceptance of ACKs not matching a
 child socket

On Thu, Dec 5, 2019 at 10:00 AM Guillaume Nault <gnault@...hat.com> wrote:
>
> On Wed, Dec 04, 2019 at 07:08:49PM -0800, Eric Dumazet wrote:
> >
> >
> > On 12/4/19 4:59 PM, Guillaume Nault wrote:
> > > When no synflood occurs, the synflood timestamp isn't updated.
> > > Therefore it can be so old that time_after32() can consider it to be
> > > in the future.
> > >
> > > That's a problem for tcp_synq_no_recent_overflow() as it may report
> > > that a recent overflow occurred while, in fact, it's just that jiffies
> > > has grown past 'last_overflow' + TCP_SYNCOOKIE_VALID + 2^31.
> > >
> > > Spurious detection of recent overflows lead to extra syncookie
> > > verification in cookie_v[46]_check(). At that point, the verification
> > > should fail and the packet dropped. But we should have dropped the
> > > packet earlier as we didn't even send a syncookie.
> > >
> > > Let's refine tcp_synq_no_recent_overflow() to report a recent overflow
> > > only if jiffies is within the
> > > [last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval. This
> > > way, no spurious recent overflow is reported when jiffies wraps and
> > > 'last_overflow' becomes in the future from the point of view of
> > > time_after32().
> > >
> > > However, if jiffies wraps and enters the
> > > [last_overflow, last_overflow + TCP_SYNCOOKIE_VALID] interval (with
> > > 'last_overflow' being a stale synflood timestamp), then
> > > tcp_synq_no_recent_overflow() still erroneously reports an
> > > overflow. In such cases, we have to rely on syncookie verification
> > > to drop the packet. We unfortunately have no way to differentiate
> > > between a fresh and a stale syncookie timestamp.
> > >
> > > Signed-off-by: Guillaume Nault <gnault@...hat.com>
> > > ---
> > >  include/net/tcp.h | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/include/net/tcp.h b/include/net/tcp.h
> > > index f0eae83ee555..005d4c691543 100644
> > > --- a/include/net/tcp.h
> > > +++ b/include/net/tcp.h
> > > @@ -520,12 +520,14 @@ static inline bool tcp_synq_no_recent_overflow(const struct sock *sk)
> > >             if (likely(reuse)) {
> > >                     last_overflow = READ_ONCE(reuse->synq_overflow_ts);
> > >                     return time_after32(now, last_overflow +
> > > -                                       TCP_SYNCOOKIE_VALID);
> > > +                                       TCP_SYNCOOKIE_VALID) ||
> > > +                           time_before32(now, last_overflow);
> > >             }
> > >     }
> > >
> > >     last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
> > > -   return time_after32(now, last_overflow + TCP_SYNCOOKIE_VALID);
> > > +   return time_after32(now, last_overflow + TCP_SYNCOOKIE_VALID) ||
> > > +           time_before32(now, last_overflow);
> > >  }
> >
> >
> > There is a race I believe here.
> >
> > CPU1                                 CPU2
> >
> > now = jiffies.
> >     ...
> >                                      jiffies++
> >                                      ...
> >                                      SYN received, last_overflow is updated to the new jiffies.
> >
> >
> > CPU1
> >  timer_before32(now, last_overflow) is true, because last_overflow was set to now+1
> >
> >
> > I suggest some cushion here.
> >
> Yes, we should wrap access to ->rx_opt.ts_recent_stamp into READ_ONCE(),
> to ensure that last_overflow won't be reloaded between the
> time_after32() and the time_before32() calls. Is that what you had in
> mind?
>
> -       last_overflow = tcp_sk(sk)->rx_opt.ts_recent_stamp;
> +       last_overflow = READ_ONCE(tcp_sk(sk)->rx_opt.ts_recent_stamp);
>
> Patch 1 would need the same fix BTW.
>
> > Also we TCP uses between() macro, we might add a time_between32(a, b, c) macro
> > to ease code review.
> >
> I didn't realise that. I'll define it in v3.
>
> > ->
> >   return !time_between32(last_overflow - HZ, now, last_overflow + TCP_SYNCOOKIE_VALID);
> >
> 'last_overflow - HZ'? I don't get why we'd take HZ into account here.
>

Please read carefuly my prior feedback.

Even with READ_ONCE(), you still have a race.


CPU1                                 CPU2

now = jiffies.
 <some long interrupt>
                                     jiffies++  (or jiffies += 3 or 4
if CPU1 has been interrupted by a long interrupt)
                                     ...
                                     SYN received, last_overflow is
updated to the new jiffies.


CPU1

 @now still has a stale values (an old jiffies value)
 timer_before32(now, last_overflow) is true, because last_overflow was
set to now+1 (or now + 2 or now + 3)