netdev - Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash

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 for Android: free password hash cracker in your pocket

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

Message-ID: <CANn89iKHBkTbT9fZ3qbfZKE25MuS=av+frnRerGvP+_gUHPSAA@mail.gmail.com>
Date:   Tue, 30 Jun 2020 16:50:12 -0700
From:   Eric Dumazet <edumazet@...gle.com>
To:     Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>
Subject: Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()

On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers
<mathieu.desnoyers@...icios.com> wrote:
>
> ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@...gle.com wrote:
>
> > MD5 keys are read with RCU protection, and tcp_md5_do_add()
> > might update in-place a prior key.
> >
> > Normally, typical RCU updates would allocate a new piece
> > of memory. In this case only key->key and key->keylen might
> > be updated, and we do not care if an incoming packet could
> > see the old key, the new one, or some intermediate value,
> > since changing the key on a live flow is known to be problematic
> > anyway.
>
> What makes it acceptable to observe an intermediate bogus key during the
> transition ?

If you change a key while packets are in flight, the result is that :

1) Either your packet has the correct key and is handled

2) Or the key do not match, packet is dropped.

 Sender will retransmit eventually.

If this was not the case, then we could not revert the patch you are
complaining about :)

>
> Thanks,
>
> Mathieu
>
> >
> > We only want to make sure that in the case key->keylen
> > is changed, cpus in tcp_md5_hash_key() wont try to use
> > uninitialized data, or crash because key->keylen was
> > read twice to feed sg_init_one() and ahash_request_set_crypt()
> >
> > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock")
> > Signed-off-by: Eric Dumazet <edumazet@...gle.com>
> > Cc: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
> > ---
> > net/ipv4/tcp.c      | 7 +++++--
> > net/ipv4/tcp_ipv4.c | 3 +++
> > 2 files changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> > index
> > 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263
> > 100644
> > --- a/net/ipv4/tcp.c
> > +++ b/net/ipv4/tcp.c
> > @@ -4033,10 +4033,13 @@ EXPORT_SYMBOL(tcp_md5_hash_skb_data);
> >
> > int tcp_md5_hash_key(struct tcp_md5sig_pool *hp, const struct tcp_md5sig_key
> > *key)
> > {
> > +     u8 keylen = key->keylen;
> >       struct scatterlist sg;
> >
> > -     sg_init_one(&sg, key->key, key->keylen);
> > -     ahash_request_set_crypt(hp->md5_req, &sg, NULL, key->keylen);
> > +     smp_rmb(); /* paired with smp_wmb() in tcp_md5_do_add() */
> > +
> > +     sg_init_one(&sg, key->key, keylen);
> > +     ahash_request_set_crypt(hp->md5_req, &sg, NULL, keylen);
> >       return crypto_ahash_update(hp->md5_req);
> > }
> > EXPORT_SYMBOL(tcp_md5_hash_key);
> > diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> > index
> > ad6435ba6d72ffd8caf783bb25cad7ec151d6909..99916fcc15ca0be12c2c133ff40516f79e6fdf7f
> > 100644
> > --- a/net/ipv4/tcp_ipv4.c
> > +++ b/net/ipv4/tcp_ipv4.c
> > @@ -1113,6 +1113,9 @@ int tcp_md5_do_add(struct sock *sk, const union
> > tcp_md5_addr *addr,
> >       if (key) {
> >               /* Pre-existing entry - just update that one. */
> >               memcpy(key->key, newkey, newkeylen);
> > +
> > +             smp_wmb(); /* pairs with smp_rmb() in tcp_md5_hash_key() */
> > +
> >               key->keylen = newkeylen;
> >               return 0;
> >       }
> > --
> > 2.27.0.212.ge8ba1cc988-goog
>
> --
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com