----- On Jul 1, 2020, at 11:50 AM, Eric Dumazet eduma...@google.com wrote:
> My prior fix went a bit too far, according to Herbert and Mathieu.
>
> Since we accept that concurrent TCP MD5 lookups might see inconsistent
> keys, we can use READ_ONCE()/WRITE_ONCE() instead of smp_rmb()/smp_wmb()
>
> Clearing all key->key[] is needed to avoid possible KMSAN reports,
> if key->keylen is increased. Since tcp_md5_do_add() is not fast path,
> using __GFP_ZERO to clear all struct tcp_md5sig_key is simpler.
>
> data_race() was added in linux-5.8 and will prevent KCSAN reports,
> this can safely be removed in stable backports, if data_race() is
> not yet backported.
>
> Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in
> tcp_md5_do_add()/tcp_md5_hash_key()")
> Signed-off-by: Eric Dumazet <eduma...@google.com>
> Cc: Mathieu Desnoyers <mathieu.desnoy...@efficios.com>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> ---
> net/ipv4/tcp.c | 4 +---
> net/ipv4/tcp_ipv4.c | 19 ++++++++++++++-----
> 2 files changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index
> f111660453241692a17c881dd6dc2910a1236263..c3af8180c7049d5c4987bf5c67e4aff2ed6967c9
> 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4033,11 +4033,9 @@ 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;
> + u8 keylen = READ_ONCE(key->keylen); /* paired with WRITE_ONCE() in
> tcp_md5_do_add */
> struct scatterlist sg;
>
> - 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);
I think we should change this to:
return data_race(crypto_ahash_update(hp->md5_req));
because both sides can race on the data. Hopefully that would let
KCSAN know that deep within the ->update callback the data race
is OK (?)
Thanks,
Mathieu
> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index
> 99916fcc15ca0be12c2c133ff40516f79e6fdf7f..04bfcbbfee83aadf5bca0332275c57113abdbc75
> 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -1111,12 +1111,21 @@ int tcp_md5_do_add(struct sock *sk, const union
> tcp_md5_addr *addr,
>
> key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen, l3index);
> if (key) {
> - /* Pre-existing entry - just update that one. */
> - memcpy(key->key, newkey, newkeylen);
> + /* Pre-existing entry - just update that one.
> + * Note that the key might be used concurrently.
> + * data_race() is telling kcsan that we do not care of
> + * key mismatches, since changing MD5 key on live flows
> + * can lead to packet drops.
> + */
> + data_race(memcpy(key->key, newkey, newkeylen));
>
> - smp_wmb(); /* pairs with smp_rmb() in tcp_md5_hash_key() */
> + /* Pairs with READ_ONCE() in tcp_md5_hash_key().
> + * Also note that a reader could catch new key->keylen value
> + * but old key->key[], this is the reason we use __GFP_ZERO
> + * at sock_kmalloc() time below these lines.
> + */
> + WRITE_ONCE(key->keylen, newkeylen);
>
> - key->keylen = newkeylen;
> return 0;
> }
>
> @@ -1132,7 +1141,7 @@ int tcp_md5_do_add(struct sock *sk, const union
> tcp_md5_addr *addr,
> rcu_assign_pointer(tp->md5sig_info, md5sig);
> }
>
> - key = sock_kmalloc(sk, sizeof(*key), gfp);
> + key = sock_kmalloc(sk, sizeof(*key), gfp | __GFP_ZERO);
> if (!key)
> return -ENOMEM;
> if (!tcp_alloc_md5sig_pool()) {
> --
> 2.27.0.212.ge8ba1cc988-goog
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
