Re: [regression] TCP_MD5SIG on established sockets
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Date: 2020-06-30 23:44:26
Also in:
lkml
----- On Jun 30, 2020, at 6:38 PM, Eric Dumazet edumazet@google.com wrote: [...]
For updates of keys, it seems existing code lacks some RCU care. MD5 keys use RCU for lookups/hashes, but the replacement of a key does not allocate a new piece of memory.
How is that RCU-safe ?
Based on what I see here:
tcp_md5_do_add() has a comment stating:
"/* This can be called on a newly created socket, from other files */"
which appears to be untrue if this can indeed be called on a live socket.
The path for pre-existing keys does:
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);
key->keylen = newkeylen;
return 0;
}
AFAIU, this works only if you assume there are no concurrent readers
accessing key->key, else they can see a corrupted key.
The change you are proposing adds smp_wmb/smp_rmb to pair stores
to key before key_len with loads of key_len before key. I'm not sure
what this is trying to achieve, and how it prevents the readers from
observing a corrupted state if the key is updated on a live socket ?
Based on my understanding, this path which deals with pre-existing keys
in-place should only ever be used when there are no concurrent readers,
else a new memory allocation would be needed to guarantee that readers
always observe a valid copy.
Thanks,
Mathieu
quoted hunk ↗ jump to hunk
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.cindex 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..ecc0e3fabce8b03bef823cbfc5c1b0a9e24df124 100644--- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c@@ -4034,9 +4034,12 @@ EXPORT_SYMBOL(tcp_md5_hash_skb_data);int tcp_md5_hash_key(struct tcp_md5sig_pool *hp, const struct tcp_md5sig_key *key) { struct scatterlist sg; + u8 keylen = key->keylen; - 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.cindex 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 uniontcp_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; }
-- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com