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]

Message-ID: <1359584061.18162.1593560822147.JavaMail.zimbra@efficios.com>
Date:   Tue, 30 Jun 2020 19:47:02 -0400 (EDT)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.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>
Subject: Re: [PATCH net] tcp: md5: add missing memory barriers in
 tcp_md5_do_add()/tcp_md5_hash_key()

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

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