Messages in this thread Patch in this message | | | From | Eric Dumazet <> | Date | Tue, 30 Jun 2020 15:38:31 -0700 | Subject | Re: [regression] TCP_MD5SIG on established sockets |
| |
On Tue, Jun 30, 2020 at 3:07 PM Eric Dumazet <edumazet@google.com> wrote: > > On Tue, Jun 30, 2020 at 2:54 PM Eric Dumazet <edumazet@google.com> wrote: > > > > On Tue, Jun 30, 2020 at 2:23 PM Eric Dumazet <edumazet@google.com> wrote: > > > > > > On Tue, Jun 30, 2020 at 2:17 PM Mathieu Desnoyers > > > <mathieu.desnoyers@efficios.com> wrote: > > > > > > > > ----- On Jun 30, 2020, at 4:56 PM, Eric Dumazet edumazet@google.com wrote: > > > > > > > > > On Tue, Jun 30, 2020 at 1:44 PM David Miller <davem@davemloft.net> wrote: > > > > >> > > > > >> From: Eric Dumazet <edumazet@google.com> > > > > >> Date: Tue, 30 Jun 2020 13:39:27 -0700 > > > > >> > > > > >> > The (C) & (B) case are certainly doable. > > > > >> > > > > > >> > A) case is more complex, I have no idea of breakages of various TCP > > > > >> > stacks if a flow got SACK > > > > >> > at some point (in 3WHS) but suddenly becomes Reno. > > > > >> > > > > >> I agree that C and B are the easiest to implement without having to > > > > >> add complicated code to handle various negotiated TCP option > > > > >> scenerios. > > > > >> > > > > >> It does seem to be that some entities do A, or did I misread your > > > > >> behavioral analysis of various implementations Mathieu? > > > > >> > > > > >> Thanks. > > > > > > > > > > Yes, another question about Mathieu cases is do determine the behavior > > > > > of all these stacks vs : > > > > > SACK option > > > > > TCP TS option. > > > > > > > > I will ask my customer's networking team to investigate these behaviors, > > > > which will allow me to prepare a thorough reply to the questions raised > > > > by Eric and David. I expect to have an answer within 2-3 weeks at most. > > > > > > > > Thank you! > > > > > > > > > Great, I am working on adding back support for (B) & (C) by the end of > > > this week. > > > > Note that the security issue (of sending uninit bytes to the wire) has > > been independently fixed with [1] > > > > This means syzbot was able to have MD5+TS+SACK ~6 months ago. > > > > It seems we (linux) do not enable this combination for PASSIVE flows, > > (according to tcp_synack_options()), > > but for ACTIVE flows we do nothing special. > > > > So maybe code in tcp_synack_options() should be mirrored to > > tcp_syn_options() for consistency. > > (disabling TS if both MD5 and SACK are enabled) > > Oh well, tcp_syn_options() is supposed to have the same logic. > > Maybe we have an issue with SYNCOOKIES (with MD5 + TS + SACK) > > Nice can of worms. >
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.
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 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.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; }
| |