----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet eduma...@google.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 <eduma...@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoy...@efficios.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
