netdev - Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

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]

Date:   Mon, 17 Dec 2018 14:01:41 -0800
From:   Eric Dumazet <edumazet@...gle.com>
To:     Christoph Paasch <cpaasch@...le.com>
Cc:     netdev <netdev@...r.kernel.org>, Yuchung Cheng <ycheng@...gle.com>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH net-next 1/5] tcp: Create list of TFO-contexts

On Mon, Dec 17, 2018 at 1:57 PM Christoph Paasch <cpaasch@...le.com> wrote:
>
> On 17/12/18 - 08:04:08, Eric Dumazet wrote:
> > On Fri, Dec 14, 2018 at 2:40 PM Christoph Paasch <cpaasch@...le.com> wrote:
> > >
> >
> > ...
> >
> > >  int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> > >                               void *key, unsigned int len)
> > >  {
> > > @@ -96,13 +131,22 @@ error:             kfree(ctx);
> > >         spin_lock(&net->ipv4.tcp_fastopen_ctx_lock);
> > >         if (sk) {
> > >                 q = &inet_csk(sk)->icsk_accept_queue.fastopenq;
> >
> > > +               rcu_assign_pointer(ctx->next, q->ctx);
> > At this point, ctx is not yet visible, so you do not need a barrier yet
> >                     ctx->next = q->ctx;
>
> Thanks, I will change that.
>
> >
> >
> > > +               rcu_assign_pointer(q->ctx, ctx);
> >
> > Note that readers could see 3 contexts in the chain, instead of maximum two.
> >
> > This means that proc_tcp_fastopen_key() (your 3/5 change) would
> > overflow an automatic array :
> >
> > while (ctxt) {
> >         memcpy(&key[i], ctxt->key, TCP_FASTOPEN_KEY_LENGTH);
> >         i += 4;
> >         ctxt = rcu_dereference(ctxt->next);
> > }
>
> Ouch! Thanks for spotting this.
>
> If it's ok to have a brief moment of 3 contexts for the readers, I would
> protect against overflows the readers.

I believe you can refactor the code here, to publish the new pointer
(rcu_assign_pointer(ctx->next, q->ctx);)
only after you have shorten the chain.

No worries if one incoming packet can see only the old primary key,
but not the fallback one,
since we are anyway about to remove the old fallback.

Ideally the rcu_assign_pointer(ctx->next, q->ctx) operation should be
the last one, when the new chain
is clean and ready to be used.