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  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:   Tue, 30 Jun 2020 21:53:40 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     Hideaki Yoshifuji <hideaki.yoshifuji@...aclelinux.com>,
        "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>
Subject: Re: [PATCH net] tcp: md5: add missing memory barriers in
 tcp_md5_do_add()/tcp_md5_hash_key()

----- On Jun 30, 2020, at 8:55 PM, Eric Dumazet edumazet@...gle.com wrote:

> On Tue, Jun 30, 2020 at 5:53 PM Hideaki Yoshifuji
> <hideaki.yoshifuji@...aclelinux.com> wrote:
>>
>> Hi,
>>
>> 2020年7月1日(水) 8:41 Eric Dumazet <edumazet@...gle.com>:
>> :
>> > 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;
>>
>> ACCESS_ONCE here, no?
> 
> Not needed, the smp_rmb() barrier is stronger.

ACCESS_ONCE() is now deprecated in favor of READ_ONCE()/WRITE_ONCE(),
which are needed here. smp_rmb()/smp_wmb() are needed in addition, but
have a different purpose. smp_rmb() only guarantees ordering, but does
nothing to prevent the compiler from turning the load into something
unexpected. Likewise for WRITE_ONCE. See linux/compiler.h READ_ONCE and
WRITE_ONCE comment:

"2) Ensuring that the compiler does not fold, spindle, or otherwise
mutilate accesses that either do not require ordering or that interact
with an explicit memory barrier or atomic instruction that provides the
required ordering."

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Powered by blists - more mailing lists