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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Jul 2020 09:52:07 +0900
From:   Hideaki Yoshifuji <hideaki.yoshifuji@...aclelinux.com>
To:     Eric Dumazet <edumazet@...gle.com>
Cc:     "David S . Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()

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?

--yoshfuji

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