| From c7bb39708d11147c451c38695e3d6162904f4fb1 Mon Sep 17 00:00:00 2001 |
| From: Eric Dumazet <edumazet@google.com> |
| Date: Wed, 1 Jul 2020 11:43:04 -0700 |
| Subject: [PATCH] tcp: md5: refine tcp_md5_do_add()/tcp_md5_hash_key() barriers |
| |
| commit e6ced831ef11a2a06e8d00aad9d4fc05b610bf38 upstream. |
| |
| 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. |
| |
| v2: use data_race() both in tcp_md5_hash_key() and tcp_md5_do_add() |
| |
| Fixes: 6a2febec338d ("tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key()") |
| Signed-off-by: Eric Dumazet <edumazet@google.com> |
| Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> |
| Cc: Herbert Xu <herbert@gondor.apana.org.au> |
| Cc: Marco Elver <elver@google.com> |
| Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> |
| Acked-by: Herbert Xu <herbert@gondor.apana.org.au> |
| Signed-off-by: David S. Miller <davem@davemloft.net> |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| [PG: use v5.4.x-stable version w/o data_race as per above info.] |
| Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com> |
| |
| diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c |
| index 18282f5ff66e..cac96434f0be 100644 |
| --- a/net/ipv4/tcp.c |
| +++ b/net/ipv4/tcp.c |
| @@ -3809,13 +3809,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; |
| + 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); |
| + |
| + /* tcp_md5_do_add() might change key->key under us */ |
| 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 4e1252674a1f..ef4aa931d044 100644 |
| --- a/net/ipv4/tcp_ipv4.c |
| +++ b/net/ipv4/tcp_ipv4.c |
| @@ -1067,12 +1067,18 @@ int tcp_md5_do_add(struct sock *sk, const union tcp_md5_addr *addr, |
| |
| key = tcp_md5_do_lookup_exact(sk, addr, family, prefixlen); |
| if (key) { |
| - /* Pre-existing entry - just update that one. */ |
| + /* Pre-existing entry - just update that one. |
| + * Note that the key might be used concurrently. |
| + */ |
| 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; |
| } |
| |
| @@ -1088,7 +1094,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 |
| |