| Message ID | 20200630234101.3259179-1-edumazet@google.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] tcp: md5: add missing memory barriers in tcp_md5_do_add()/tcp_md5_hash_key() | expand |
----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@google.com wrote: > MD5 keys are read with RCU protection, and tcp_md5_do_add() > might update in-place a prior key. > > Normally, typical RCU updates would allocate a new piece > of memory. In this case only key->key and key->keylen might > be updated, and we do not care if an incoming packet could > see the old key, the new one, or some intermediate value, > since changing the key on a live flow is known to be problematic > anyway. What makes it acceptable to observe an intermediate bogus key during the transition ? Thanks, Mathieu > > We only want to make sure that in the case key->keylen > is changed, cpus in tcp_md5_hash_key() wont try to use > uninitialized data, or crash because key->keylen was > read twice to feed sg_init_one() and ahash_request_set_crypt() > > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > --- > net/ipv4/tcp.c | 7 +++++-- > net/ipv4/tcp_ipv4.c | 3 +++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index > 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 > 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4033,10 +4033,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; > struct scatterlist sg; > > - 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; > } > -- > 2.27.0.212.ge8ba1cc988-goog
On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@google.com wrote: > > > MD5 keys are read with RCU protection, and tcp_md5_do_add() > > might update in-place a prior key. > > > > Normally, typical RCU updates would allocate a new piece > > of memory. In this case only key->key and key->keylen might > > be updated, and we do not care if an incoming packet could > > see the old key, the new one, or some intermediate value, > > since changing the key on a live flow is known to be problematic > > anyway. > > What makes it acceptable to observe an intermediate bogus key during the > transition ? If you change a key while packets are in flight, the result is that : 1) Either your packet has the correct key and is handled 2) Or the key do not match, packet is dropped. Sender will retransmit eventually. If this was not the case, then we could not revert the patch you are complaining about :) > > Thanks, > > Mathieu > > > > > We only want to make sure that in the case key->keylen > > is changed, cpus in tcp_md5_hash_key() wont try to use > > uninitialized data, or crash because key->keylen was > > read twice to feed sg_init_one() and ahash_request_set_crypt() > > > > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > --- > > net/ipv4/tcp.c | 7 +++++-- > > net/ipv4/tcp_ipv4.c | 3 +++ > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index > > 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 > > 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -4033,10 +4033,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; > > struct scatterlist sg; > > > > - 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; > > } > > -- > > 2.27.0.212.ge8ba1cc988-goog > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com
----- On Jun 30, 2020, at 7:50 PM, Eric Dumazet edumazet@google.com wrote: > On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: >> >> ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@google.com wrote: >> >> > MD5 keys are read with RCU protection, and tcp_md5_do_add() >> > might update in-place a prior key. >> > >> > Normally, typical RCU updates would allocate a new piece >> > of memory. In this case only key->key and key->keylen might >> > be updated, and we do not care if an incoming packet could >> > see the old key, the new one, or some intermediate value, >> > since changing the key on a live flow is known to be problematic >> > anyway. >> >> What makes it acceptable to observe an intermediate bogus key during the >> transition ? > > If you change a key while packets are in flight, the result is that : > > 1) Either your packet has the correct key and is handled > > 2) Or the key do not match, packet is dropped. > > Sender will retransmit eventually. This train of thoughts seem to apply to incoming traffic, what about outgoing ? > > If this was not the case, then we could not revert the patch you are > complaining about :) Please let me know where I'm incorrect with the following scenario: - Local peer is a Linux host, which supports only a single MD5 key per socket at any given time. - Remote peer is a Ericsson/Redback device allowing 2 passwords (old/new) to co-exist until both sides are OK with the new password. The local peer updates the MD5 password for a socket in parallel with packets flow. If we guarantee that no intermediate bogus key state is observable, the flow going out of the Linux peer should always be seen as valid, with either the old or the new key. Allowing an intermediate bogus key to be observable means this introduces a race condition during which the remote peer can observe a corrupted key. Thanks, Mathieu
On Tue, Jun 30, 2020 at 5:27 PM Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote: > > ----- On Jun 30, 2020, at 7:50 PM, Eric Dumazet edumazet@google.com wrote: > > > On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers > > <mathieu.desnoyers@efficios.com> wrote: > >> > >> ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@google.com wrote: > >> > >> > MD5 keys are read with RCU protection, and tcp_md5_do_add() > >> > might update in-place a prior key. > >> > > >> > Normally, typical RCU updates would allocate a new piece > >> > of memory. In this case only key->key and key->keylen might > >> > be updated, and we do not care if an incoming packet could > >> > see the old key, the new one, or some intermediate value, > >> > since changing the key on a live flow is known to be problematic > >> > anyway. > >> > >> What makes it acceptable to observe an intermediate bogus key during the > >> transition ? > > > > If you change a key while packets are in flight, the result is that : > > > > 1) Either your packet has the correct key and is handled > > > > 2) Or the key do not match, packet is dropped. > > > > Sender will retransmit eventually. > > This train of thoughts seem to apply to incoming traffic, what about outgoing ? Outgoing path is protected by the socket lock. You can not change the TCP MD5 key while xmit is in progress. > > > > > If this was not the case, then we could not revert the patch you are > > complaining about :) > > Please let me know where I'm incorrect with the following scenario: > > - Local peer is a Linux host, which supports only a single MD5 key > per socket at any given time. > - Remote peer is a Ericsson/Redback device allowing 2 passwords (old/new) > to co-exist until both sides are OK with the new password. > > The local peer updates the MD5 password for a socket in parallel with > packets flow. If we guarantee that no intermediate bogus key state is > observable, the flow going out of the Linux peer should always be seen as > valid, with either the old or the new key. Linux has one key. If your peer sends a packet with the old key, you will drop the packet, no matter how hard you try. > > Allowing an intermediate bogus key to be observable means this introduces > a race condition during which the remote peer can observe a corrupted key. Race condition is there, even if you change the whole TCP MD5 key atomically. If another cpu receives a packet while you are changing the key, you definitely can not predict if the packet is going to be dropped or not. > > Thanks, > > Mathieu > > -- > Mathieu Desnoyers > EfficiOS Inc. > http://www.efficios.com
----- On Jun 30, 2020, at 8:34 PM, Eric Dumazet edumazet@google.com wrote: > On Tue, Jun 30, 2020 at 5:27 PM Mathieu Desnoyers > <mathieu.desnoyers@efficios.com> wrote: >> >> ----- On Jun 30, 2020, at 7:50 PM, Eric Dumazet edumazet@google.com wrote: >> >> > On Tue, Jun 30, 2020 at 4:47 PM Mathieu Desnoyers >> > <mathieu.desnoyers@efficios.com> wrote: >> >> >> >> ----- On Jun 30, 2020, at 7:41 PM, Eric Dumazet edumazet@google.com wrote: >> >> >> >> > MD5 keys are read with RCU protection, and tcp_md5_do_add() >> >> > might update in-place a prior key. >> >> > >> >> > Normally, typical RCU updates would allocate a new piece >> >> > of memory. In this case only key->key and key->keylen might >> >> > be updated, and we do not care if an incoming packet could >> >> > see the old key, the new one, or some intermediate value, >> >> > since changing the key on a live flow is known to be problematic >> >> > anyway. >> >> >> >> What makes it acceptable to observe an intermediate bogus key during the >> >> transition ? >> > >> > If you change a key while packets are in flight, the result is that : >> > >> > 1) Either your packet has the correct key and is handled >> > >> > 2) Or the key do not match, packet is dropped. >> > >> > Sender will retransmit eventually. >> >> This train of thoughts seem to apply to incoming traffic, what about outgoing ? > > > Outgoing path is protected by the socket lock. > > You can not change the TCP MD5 key while xmit is in progress. Allright, this is the part I missed, thanks! Mathieu
Hi, 2020年7月1日(水) 8:41 Eric Dumazet <edumazet@google.com>: : > We only want to make sure that in the case key->keylen > is changed, cpus in tcp_md5_hash_key() wont try to use > uninitialized data, or crash because key->keylen was > read twice to feed sg_init_one() and ahash_request_set_crypt() > > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > --- > net/ipv4/tcp.c | 7 +++++-- > net/ipv4/tcp_ipv4.c | 3 +++ > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > index 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 100644 > --- a/net/ipv4/tcp.c > +++ b/net/ipv4/tcp.c > @@ -4033,10 +4033,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; > struct scatterlist sg; ACCESS_ONCE here, no? --yoshfuji > > - 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; > } > -- > 2.27.0.212.ge8ba1cc988-goog >
On Tue, Jun 30, 2020 at 5:53 PM Hideaki Yoshifuji <hideaki.yoshifuji@miraclelinux.com> wrote: > > Hi, > > 2020年7月1日(水) 8:41 Eric Dumazet <edumazet@google.com>: > : > > We only want to make sure that in the case key->keylen > > is changed, cpus in tcp_md5_hash_key() wont try to use > > uninitialized data, or crash because key->keylen was > > read twice to feed sg_init_one() and ahash_request_set_crypt() > > > > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") > > Signed-off-by: Eric Dumazet <edumazet@google.com> > > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> > > --- > > net/ipv4/tcp.c | 7 +++++-- > > net/ipv4/tcp_ipv4.c | 3 +++ > > 2 files changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c > > index 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 100644 > > --- a/net/ipv4/tcp.c > > +++ b/net/ipv4/tcp.c > > @@ -4033,10 +4033,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; > > struct scatterlist sg; > > ACCESS_ONCE here, no? Not needed, the smp_rmb() barrier is stronger.
From: Eric Dumazet <edumazet@google.com> Date: Tue, 30 Jun 2020 16:41:01 -0700 > MD5 keys are read with RCU protection, and tcp_md5_do_add() > might update in-place a prior key. > > Normally, typical RCU updates would allocate a new piece > of memory. In this case only key->key and key->keylen might > be updated, and we do not care if an incoming packet could > see the old key, the new one, or some intermediate value, > since changing the key on a live flow is known to be problematic > anyway. > > We only want to make sure that in the case key->keylen > is changed, cpus in tcp_md5_hash_key() wont try to use > uninitialized data, or crash because key->keylen was > read twice to feed sg_init_one() and ahash_request_set_crypt() > > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") > Signed-off-by: Eric Dumazet <edumazet@google.com> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Applied and queued up for -stable, thanks Eric.
----- On Jun 30, 2020, at 8:55 PM, Eric Dumazet edumazet@google.com wrote: > On Tue, Jun 30, 2020 at 5:53 PM Hideaki Yoshifuji > <hideaki.yoshifuji@miraclelinux.com> wrote: >> >> Hi, >> >> 2020年7月1日(水) 8:41 Eric Dumazet <edumazet@google.com>: >> : >> > We only want to make sure that in the case key->keylen >> > is changed, cpus in tcp_md5_hash_key() wont try to use >> > uninitialized data, or crash because key->keylen was >> > read twice to feed sg_init_one() and ahash_request_set_crypt() >> > >> > Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") >> > Signed-off-by: Eric Dumazet <edumazet@google.com> >> > Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> >> > --- >> > net/ipv4/tcp.c | 7 +++++-- >> > net/ipv4/tcp_ipv4.c | 3 +++ >> > 2 files changed, 8 insertions(+), 2 deletions(-) >> > >> > diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c >> > index >> > 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 >> > 100644 >> > --- a/net/ipv4/tcp.c >> > +++ b/net/ipv4/tcp.c >> > @@ -4033,10 +4033,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; >> > struct scatterlist sg; >> >> ACCESS_ONCE here, no? > > Not needed, the smp_rmb() barrier is stronger. ACCESS_ONCE() is now deprecated in favor of READ_ONCE()/WRITE_ONCE(), which are needed here. smp_rmb()/smp_wmb() are needed in addition, but have a different purpose. smp_rmb() only guarantees ordering, but does nothing to prevent the compiler from turning the load into something unexpected. Likewise for WRITE_ONCE. See linux/compiler.h READ_ONCE and WRITE_ONCE comment: "2) Ensuring that the compiler does not fold, spindle, or otherwise mutilate accesses that either do not require ordering or that interact with an explicit memory barrier or atomic instruction that provides the required ordering." Thanks, Mathieu
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c index 810cc164f795f8e1e8ca747ed5df51bb20fec8a2..f111660453241692a17c881dd6dc2910a1236263 100644 --- a/net/ipv4/tcp.c +++ b/net/ipv4/tcp.c @@ -4033,10 +4033,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; struct scatterlist sg; - 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; }
MD5 keys are read with RCU protection, and tcp_md5_do_add() might update in-place a prior key. Normally, typical RCU updates would allocate a new piece of memory. In this case only key->key and key->keylen might be updated, and we do not care if an incoming packet could see the old key, the new one, or some intermediate value, since changing the key on a live flow is known to be problematic anyway. We only want to make sure that in the case key->keylen is changed, cpus in tcp_md5_hash_key() wont try to use uninitialized data, or crash because key->keylen was read twice to feed sg_init_one() and ahash_request_set_crypt() Fixes: 9ea88a153001 ("tcp: md5: check md5 signature without socket lock") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> --- net/ipv4/tcp.c | 7 +++++-- net/ipv4/tcp_ipv4.c | 3 +++ 2 files changed, 8 insertions(+), 2 deletions(-)