From: Eric Dumazet <edumazet@google.com>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: "David S. Miller" <davem@davemloft.net>,
Linus Torvalds <torvalds@linux-foundation.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
netdev <netdev@vger.kernel.org>,
Yuchung Cheng <ycheng@google.com>,
Jonathan Rajotte-Julien <joraj@efficios.com>
Subject: Re: [regression] TCP_MD5SIG on established sockets
Date: Tue, 30 Jun 2020 15:38:31 -0700 [thread overview]
Message-ID: <CANn89iLPqtJG0iESCHF+RcOjo95ukan1oSzjkPjoSJgKpO2wSQ@mail.gmail.com> (raw)
In-Reply-To: <CANn89iJJ_WR-jGQogU3-arjD6=xcU9VWzJYSOLbyD94JQo-zAQ@mail.gmail.com>
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;
}
next prev parent reply other threads:[~2020-06-30 22:38 UTC|newest]
Thread overview: 32+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-13 19:38 [regression] TC_MD5SIG on established sockets Mathieu Desnoyers
2020-05-13 19:49 ` Eric Dumazet
2020-05-13 19:56 ` Eric Dumazet
2020-06-29 19:43 ` [regression] TCP_MD5SIG " Mathieu Desnoyers
2020-06-29 20:47 ` Eric Dumazet
2020-06-30 19:43 ` Linus Torvalds
2020-06-30 19:52 ` Linus Torvalds
2020-06-30 20:34 ` Mathieu Desnoyers
2020-06-30 20:39 ` Eric Dumazet
2020-06-30 20:44 ` David Miller
2020-06-30 20:56 ` Eric Dumazet
2020-06-30 21:17 ` Mathieu Desnoyers
2020-06-30 21:23 ` Eric Dumazet
2020-06-30 21:54 ` Eric Dumazet
2020-06-30 22:07 ` Eric Dumazet
2020-06-30 22:38 ` Eric Dumazet [this message]
2020-06-30 23:44 ` Mathieu Desnoyers
2020-07-01 0:01 ` Eric Dumazet
2020-07-01 2:02 ` Herbert Xu
2020-07-01 2:17 ` Eric Dumazet
2020-07-01 2:22 ` Herbert Xu
2020-07-01 2:30 ` Eric Dumazet
2020-07-01 2:39 ` Joe Perches
2020-07-01 2:58 ` Herbert Xu
2020-07-01 3:36 ` Eric Dumazet
2020-07-01 3:50 ` Herbert Xu
2020-07-01 12:19 ` Mathieu Desnoyers
2020-07-01 15:15 ` Eric Dumazet
2020-07-01 17:24 ` Eric Dumazet
2020-06-30 20:21 ` David Miller
2020-06-30 20:30 ` Eric Dumazet
2020-06-30 20:39 ` Mathieu Desnoyers
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANn89iLPqtJG0iESCHF+RcOjo95ukan1oSzjkPjoSJgKpO2wSQ@mail.gmail.com \
--to=edumazet@google.com \
--cc=davem@davemloft.net \
--cc=joraj@efficios.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=netdev@vger.kernel.org \
--cc=torvalds@linux-foundation.org \
--cc=ycheng@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).