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: 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