Netdev Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch
@ 2019-06-18  9:32 Ard Biesheuvel
  2019-06-18  9:32 ` [PATCH 1/2] net: fastopen: make key handling more robust against future changes Ard Biesheuvel
                   ` (2 more replies)
  0 siblings, 3 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18  9:32 UTC (permalink / raw)
  To: netdev
  Cc: Ard Biesheuvel, Eric Biggers, linux-crypto, herbert, edumazet,
	davem, kuznet, yoshfuji, jbaron, cpaasch, David.Laight, ycheng

A pair of tweaks for issues spotted by Eric Biggers. Patch #1 is
mostly cosmetic, since the error check it adds is unreachable in
practice, and the other changes are syntactic cleanups. Patch #2
adds endian swabbing of the SipHash output for big endian systems
so that the in-memory representation is the same as on little
endian systems.

cc: Eric Biggers <ebiggers@kernel.org>
cc: linux-crypto@vger.kernel.org
cc: herbert@gondor.apana.org.au
cc: edumazet@google.com
cc: davem@davemloft.net
cc: kuznet@ms2.inr.ac.ru
cc: yoshfuji@linux-ipv6.org
cc: jbaron@akamai.com
cc: cpaasch@apple.com
cc: David.Laight@aculab.com
cc: ycheng@google.com

Ard Biesheuvel (2):
  net: fastopen: make key handling more robust against future changes
  net: fastopen: use endianness agnostic representation of the cookie

 include/linux/tcp.h     |  2 +-
 include/net/tcp.h       |  5 +--
 net/ipv4/tcp_fastopen.c | 34 +++++++++++---------
 3 files changed, 23 insertions(+), 18 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:32 [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Ard Biesheuvel
@ 2019-06-18  9:32 ` Ard Biesheuvel
  2019-06-18  9:39   ` Eric Dumazet
  2019-06-18  9:32 ` [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie Ard Biesheuvel
  2019-06-18  9:37 ` [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Eric Dumazet
  2 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18  9:32 UTC (permalink / raw)
  To: netdev
  Cc: Ard Biesheuvel, Eric Biggers, linux-crypto, herbert, edumazet,
	davem, kuznet, yoshfuji, jbaron, cpaasch, David.Laight, ycheng

Some changes to the TCP fastopen code to make it more robust
against future changes in the choice of key/cookie size, etc.

- Instead of keeping the SipHash key in an untyped u8[] buffer
  and casting it to the right type upon use, use the correct
  siphash_key_t type directly. This ensures that the key will
  appear at the correct alignment if we ever change the way
  these data structures are allocated. (Currently, they are
  only allocated via kmalloc so they always appear at the
  correct alignment)

- Use DIV_ROUND_UP when sizing the u64[] array to hold the
  cookie, so it is always of sufficient size, even when
  TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.

- Add a key length check to tcp_fastopen_reset_cipher(). No
  callers exist currently that fail this check (they all pass
  compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
  but future changes might create problems, e.g., by leaving part
  of the key uninitialized, or overflowing the key buffers.

Note that none of these are functional changes wrt the current
state of the code.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/tcp.h     |  2 +-
 include/net/tcp.h       |  5 +++--
 net/ipv4/tcp_fastopen.c | 22 ++++++++++++--------
 3 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 2689b0b0b68a..3d3659c638a6 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -58,7 +58,7 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 
 /* TCP Fast Open Cookie as stored in memory */
 struct tcp_fastopen_cookie {
-	u64	val[TCP_FASTOPEN_COOKIE_MAX / sizeof(u64)];
+	u64	val[DIV_ROUND_UP(TCP_FASTOPEN_COOKIE_MAX, sizeof(u64))];
 	s8	len;
 	bool	exp;	/* In RFC6994 experimental option format */
 };
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 573c9e9b0d72..9456b0834e21 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -43,6 +43,7 @@
 #include <linux/seq_file.h>
 #include <linux/memcontrol.h>
 #include <linux/bpf-cgroup.h>
+#include <linux/siphash.h>
 
 extern struct inet_hashinfo tcp_hashinfo;
 
@@ -1623,14 +1624,14 @@ void tcp_fastopen_init_key_once(struct net *net);
 bool tcp_fastopen_cookie_check(struct sock *sk, u16 *mss,
 			     struct tcp_fastopen_cookie *cookie);
 bool tcp_fastopen_defer_connect(struct sock *sk, int *err);
-#define TCP_FASTOPEN_KEY_LENGTH 16
+#define TCP_FASTOPEN_KEY_LENGTH sizeof(siphash_key_t)
 #define TCP_FASTOPEN_KEY_MAX 2
 #define TCP_FASTOPEN_KEY_BUF_LENGTH \
 	(TCP_FASTOPEN_KEY_LENGTH * TCP_FASTOPEN_KEY_MAX)
 
 /* Fastopen key context */
 struct tcp_fastopen_context {
-	__u8		key[TCP_FASTOPEN_KEY_MAX][TCP_FASTOPEN_KEY_LENGTH];
+	siphash_key_t	key[TCP_FASTOPEN_KEY_MAX];
 	int		num;
 	struct rcu_head	rcu;
 };
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 46b67128e1ca..61c15c3d3584 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -7,7 +7,6 @@
 #include <linux/tcp.h>
 #include <linux/rcupdate.h>
 #include <linux/rculist.h>
-#include <linux/siphash.h>
 #include <net/inetpeer.h>
 #include <net/tcp.h>
 
@@ -81,9 +80,15 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
 		goto out;
 	}
 
-	memcpy(ctx->key[0], primary_key, len);
+	if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
+		pr_err("TCP: TFO key length %u invalid\n", len);
+		err = -EINVAL;
+		goto out;
+	}
+
+	memcpy(&ctx->key[0], primary_key, len);
 	if (backup_key) {
-		memcpy(ctx->key[1], backup_key, len);
+		memcpy(&ctx->key[1], backup_key, len);
 		ctx->num = 2;
 	} else {
 		ctx->num = 1;
@@ -110,10 +115,9 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
 
 static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
 					     struct sk_buff *syn,
-					     const u8 *key,
+					     const siphash_key_t *key,
 					     struct tcp_fastopen_cookie *foc)
 {
-	BUILD_BUG_ON(TCP_FASTOPEN_KEY_LENGTH != sizeof(siphash_key_t));
 	BUILD_BUG_ON(TCP_FASTOPEN_COOKIE_SIZE != sizeof(u64));
 
 	if (req->rsk_ops->family == AF_INET) {
@@ -122,7 +126,7 @@ static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
 		foc->val[0] = siphash(&iph->saddr,
 				      sizeof(iph->saddr) +
 				      sizeof(iph->daddr),
-				      (const siphash_key_t *)key);
+				      key);
 		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
 		return true;
 	}
@@ -133,7 +137,7 @@ static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
 		foc->val[0] = siphash(&ip6h->saddr,
 				      sizeof(ip6h->saddr) +
 				      sizeof(ip6h->daddr),
-				      (const siphash_key_t *)key);
+				      key);
 		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
 		return true;
 	}
@@ -154,7 +158,7 @@ static void tcp_fastopen_cookie_gen(struct sock *sk,
 	rcu_read_lock();
 	ctx = tcp_fastopen_get_ctx(sk);
 	if (ctx)
-		__tcp_fastopen_cookie_gen_cipher(req, syn, ctx->key[0], foc);
+		__tcp_fastopen_cookie_gen_cipher(req, syn, &ctx->key[0], foc);
 	rcu_read_unlock();
 }
 
@@ -218,7 +222,7 @@ static int tcp_fastopen_cookie_gen_check(struct sock *sk,
 	if (!ctx)
 		goto out;
 	for (i = 0; i < tcp_fastopen_context_len(ctx); i++) {
-		__tcp_fastopen_cookie_gen_cipher(req, syn, ctx->key[i], foc);
+		__tcp_fastopen_cookie_gen_cipher(req, syn, &ctx->key[i], foc);
 		if (tcp_fastopen_cookie_match(foc, orig)) {
 			ret = i + 1;
 			goto out;
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie
  2019-06-18  9:32 [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Ard Biesheuvel
  2019-06-18  9:32 ` [PATCH 1/2] net: fastopen: make key handling more robust against future changes Ard Biesheuvel
@ 2019-06-18  9:32 ` Ard Biesheuvel
  2019-06-18 18:22   ` Eric Biggers
  2019-06-18  9:37 ` [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Eric Dumazet
  2 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18  9:32 UTC (permalink / raw)
  To: netdev
  Cc: Ard Biesheuvel, Eric Biggers, linux-crypto, herbert, edumazet,
	davem, kuznet, yoshfuji, jbaron, cpaasch, David.Laight, ycheng

Use an explicit little endian representation of the fastopen
cookie, so that the value no longer depends on the endianness
of the system. This fixes a theoretical issue only, since
fastopen keys are unlikely to be shared across load balancing
server farms that are mixed in endiannes, but it might pop up
in validation/selftests as well, so let's just settle on little
endian across the board.

Note that this change only affects big endian systems.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 include/linux/tcp.h     |  2 +-
 net/ipv4/tcp_fastopen.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index 3d3659c638a6..f3a85a7fb4b1 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -58,7 +58,7 @@ static inline unsigned int tcp_optlen(const struct sk_buff *skb)
 
 /* TCP Fast Open Cookie as stored in memory */
 struct tcp_fastopen_cookie {
-	u64	val[DIV_ROUND_UP(TCP_FASTOPEN_COOKIE_MAX, sizeof(u64))];
+	__le64	val[DIV_ROUND_UP(TCP_FASTOPEN_COOKIE_MAX, sizeof(u64))];
 	s8	len;
 	bool	exp;	/* In RFC6994 experimental option format */
 };
diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
index 61c15c3d3584..2704441a0bb0 100644
--- a/net/ipv4/tcp_fastopen.c
+++ b/net/ipv4/tcp_fastopen.c
@@ -123,10 +123,10 @@ static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
 	if (req->rsk_ops->family == AF_INET) {
 		const struct iphdr *iph = ip_hdr(syn);
 
-		foc->val[0] = siphash(&iph->saddr,
-				      sizeof(iph->saddr) +
-				      sizeof(iph->daddr),
-				      key);
+		foc->val[0] = cpu_to_le64(siphash(&iph->saddr,
+					  sizeof(iph->saddr) +
+					  sizeof(iph->daddr),
+					  key));
 		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
 		return true;
 	}
@@ -134,10 +134,10 @@ static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
 	if (req->rsk_ops->family == AF_INET6) {
 		const struct ipv6hdr *ip6h = ipv6_hdr(syn);
 
-		foc->val[0] = siphash(&ip6h->saddr,
-				      sizeof(ip6h->saddr) +
-				      sizeof(ip6h->daddr),
-				      key);
+		foc->val[0] = cpu_to_le64(siphash(&ip6h->saddr,
+					  sizeof(ip6h->saddr) +
+					  sizeof(ip6h->daddr),
+					  key));
 		foc->len = TCP_FASTOPEN_COOKIE_SIZE;
 		return true;
 	}
-- 
2.17.1


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch
  2019-06-18  9:32 [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Ard Biesheuvel
  2019-06-18  9:32 ` [PATCH 1/2] net: fastopen: make key handling more robust against future changes Ard Biesheuvel
  2019-06-18  9:32 ` [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie Ard Biesheuvel
@ 2019-06-18  9:37 ` Eric Dumazet
  2019-06-18  9:38   ` Ard Biesheuvel
  2 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-06-18  9:37 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: netdev, Eric Biggers, linux-crypto, Herbert Xu, David Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> A pair of tweaks for issues spotted by Eric Biggers. Patch #1 is
> mostly cosmetic, since the error check it adds is unreachable in
> practice, and the other changes are syntactic cleanups. Patch #2
> adds endian swabbing of the SipHash output for big endian systems
> so that the in-memory representation is the same as on little
> endian systems.
>

Please always add net or net-next in your patches for netdev@

( Documentation/networking/netdev-FAQ.rst )

Thanks.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch
  2019-06-18  9:37 ` [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Eric Dumazet
@ 2019-06-18  9:38   ` Ard Biesheuvel
  0 siblings, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18  9:38 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, Eric Biggers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, 18 Jun 2019 at 11:37, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > A pair of tweaks for issues spotted by Eric Biggers. Patch #1 is
> > mostly cosmetic, since the error check it adds is unreachable in
> > practice, and the other changes are syntactic cleanups. Patch #2
> > adds endian swabbing of the SipHash output for big endian systems
> > so that the in-memory representation is the same as on little
> > endian systems.
> >
>
> Please always add net or net-next in your patches for netdev@
>
> ( Documentation/networking/netdev-FAQ.rst )
>

Apologies. These patches are intended for net-next

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:32 ` [PATCH 1/2] net: fastopen: make key handling more robust against future changes Ard Biesheuvel
@ 2019-06-18  9:39   ` Eric Dumazet
  2019-06-18  9:41     ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Dumazet @ 2019-06-18  9:39 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: netdev, Eric Biggers, linux-crypto, Herbert Xu, David Miller,
	Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> Some changes to the TCP fastopen code to make it more robust
> against future changes in the choice of key/cookie size, etc.
>
> - Instead of keeping the SipHash key in an untyped u8[] buffer
>   and casting it to the right type upon use, use the correct
>   siphash_key_t type directly. This ensures that the key will
>   appear at the correct alignment if we ever change the way
>   these data structures are allocated. (Currently, they are
>   only allocated via kmalloc so they always appear at the
>   correct alignment)
>
> - Use DIV_ROUND_UP when sizing the u64[] array to hold the
>   cookie, so it is always of sufficient size, even when
>   TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.
>
> - Add a key length check to tcp_fastopen_reset_cipher(). No
>   callers exist currently that fail this check (they all pass
>   compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
>   but future changes might create problems, e.g., by leaving part
>   of the key uninitialized, or overflowing the key buffers.
>
> Note that none of these are functional changes wrt the current
> state of the code.
>
...

> -       memcpy(ctx->key[0], primary_key, len);
> +       if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
> +               pr_err("TCP: TFO key length %u invalid\n", len);
> +               err = -EINVAL;
> +               goto out;
> +       }


Why a pr_err() is there ?

Can unpriv users flood the syslog ?

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:39   ` Eric Dumazet
@ 2019-06-18  9:41     ` Ard Biesheuvel
  2019-06-18  9:53       ` Eric Dumazet
  0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18  9:41 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, Eric Biggers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, 18 Jun 2019 at 11:39, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > Some changes to the TCP fastopen code to make it more robust
> > against future changes in the choice of key/cookie size, etc.
> >
> > - Instead of keeping the SipHash key in an untyped u8[] buffer
> >   and casting it to the right type upon use, use the correct
> >   siphash_key_t type directly. This ensures that the key will
> >   appear at the correct alignment if we ever change the way
> >   these data structures are allocated. (Currently, they are
> >   only allocated via kmalloc so they always appear at the
> >   correct alignment)
> >
> > - Use DIV_ROUND_UP when sizing the u64[] array to hold the
> >   cookie, so it is always of sufficient size, even when
> >   TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.
> >
> > - Add a key length check to tcp_fastopen_reset_cipher(). No
> >   callers exist currently that fail this check (they all pass
> >   compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
> >   but future changes might create problems, e.g., by leaving part
> >   of the key uninitialized, or overflowing the key buffers.
> >
> > Note that none of these are functional changes wrt the current
> > state of the code.
> >
> ...
>
> > -       memcpy(ctx->key[0], primary_key, len);
> > +       if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
> > +               pr_err("TCP: TFO key length %u invalid\n", len);
> > +               err = -EINVAL;
> > +               goto out;
> > +       }
>
>
> Why a pr_err() is there ?
>
> Can unpriv users flood the syslog ?

They can if they could do so before: there was a call to
crypto_cipher_setkey() in the original pre-SipHash code which would
also result in a pr_err() on an invalid key length. That call got
removed along with the AES cipher handling, and this basically
reinstates it, as suggested by EricB.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:41     ` Ard Biesheuvel
@ 2019-06-18  9:53       ` Eric Dumazet
  2019-06-18 10:02         ` Ard Biesheuvel
  2019-06-18 18:18         ` Eric Biggers
  0 siblings, 2 replies; 14+ messages in thread
From: Eric Dumazet @ 2019-06-18  9:53 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: netdev, Eric Biggers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, Jun 18, 2019 at 2:41 AM Ard Biesheuvel
<ard.biesheuvel@linaro.org> wrote:
>
> On Tue, 18 Jun 2019 at 11:39, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
> > <ard.biesheuvel@linaro.org> wrote:
> > >
> > > Some changes to the TCP fastopen code to make it more robust
> > > against future changes in the choice of key/cookie size, etc.
> > >
> > > - Instead of keeping the SipHash key in an untyped u8[] buffer
> > >   and casting it to the right type upon use, use the correct
> > >   siphash_key_t type directly. This ensures that the key will
> > >   appear at the correct alignment if we ever change the way
> > >   these data structures are allocated. (Currently, they are
> > >   only allocated via kmalloc so they always appear at the
> > >   correct alignment)
> > >
> > > - Use DIV_ROUND_UP when sizing the u64[] array to hold the
> > >   cookie, so it is always of sufficient size, even when
> > >   TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.
> > >
> > > - Add a key length check to tcp_fastopen_reset_cipher(). No
> > >   callers exist currently that fail this check (they all pass
> > >   compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
> > >   but future changes might create problems, e.g., by leaving part
> > >   of the key uninitialized, or overflowing the key buffers.
> > >
> > > Note that none of these are functional changes wrt the current
> > > state of the code.
> > >
> > ...
> >
> > > -       memcpy(ctx->key[0], primary_key, len);
> > > +       if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
> > > +               pr_err("TCP: TFO key length %u invalid\n", len);
> > > +               err = -EINVAL;
> > > +               goto out;
> > > +       }
> >
> >
> > Why a pr_err() is there ?
> >
> > Can unpriv users flood the syslog ?
>
> They can if they could do so before: there was a call to
> crypto_cipher_setkey() in the original pre-SipHash code which would
> also result in a pr_err() on an invalid key length. That call got
> removed along with the AES cipher handling, and this basically
> reinstates it, as suggested by EricB.

This tcp_fastopen_reset_cipher() function is internal to TCP stack, all callers
always pass the correct length.

We could add checks all over the place, and end up having a TCP stack
full of defensive
checks and 10,000 additional lines of code :/

I would prefer not reinstating this.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:53       ` Eric Dumazet
@ 2019-06-18 10:02         ` Ard Biesheuvel
  2019-06-18 18:18         ` Eric Biggers
  1 sibling, 0 replies; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18 10:02 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, Eric Biggers,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, 18 Jun 2019 at 11:53, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Jun 18, 2019 at 2:41 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Tue, 18 Jun 2019 at 11:39, Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > Some changes to the TCP fastopen code to make it more robust
> > > > against future changes in the choice of key/cookie size, etc.
> > > >
> > > > - Instead of keeping the SipHash key in an untyped u8[] buffer
> > > >   and casting it to the right type upon use, use the correct
> > > >   siphash_key_t type directly. This ensures that the key will
> > > >   appear at the correct alignment if we ever change the way
> > > >   these data structures are allocated. (Currently, they are
> > > >   only allocated via kmalloc so they always appear at the
> > > >   correct alignment)
> > > >
> > > > - Use DIV_ROUND_UP when sizing the u64[] array to hold the
> > > >   cookie, so it is always of sufficient size, even when
> > > >   TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.
> > > >
> > > > - Add a key length check to tcp_fastopen_reset_cipher(). No
> > > >   callers exist currently that fail this check (they all pass
> > > >   compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
> > > >   but future changes might create problems, e.g., by leaving part
> > > >   of the key uninitialized, or overflowing the key buffers.
> > > >
> > > > Note that none of these are functional changes wrt the current
> > > > state of the code.
> > > >
> > > ...
> > >
> > > > -       memcpy(ctx->key[0], primary_key, len);
> > > > +       if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
> > > > +               pr_err("TCP: TFO key length %u invalid\n", len);
> > > > +               err = -EINVAL;
> > > > +               goto out;
> > > > +       }
> > >
> > >
> > > Why a pr_err() is there ?
> > >
> > > Can unpriv users flood the syslog ?
> >
> > They can if they could do so before: there was a call to
> > crypto_cipher_setkey() in the original pre-SipHash code which would
> > also result in a pr_err() on an invalid key length. That call got
> > removed along with the AES cipher handling, and this basically
> > reinstates it, as suggested by EricB.
>
> This tcp_fastopen_reset_cipher() function is internal to TCP stack, all callers
> always pass the correct length.
>
> We could add checks all over the place, and end up having a TCP stack
> full of defensive
> checks and 10,000 additional lines of code :/
>
> I would prefer not reinstating this.

Fair enough.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18  9:53       ` Eric Dumazet
  2019-06-18 10:02         ` Ard Biesheuvel
@ 2019-06-18 18:18         ` Eric Biggers
  2019-06-18 18:19           ` Eric Dumazet
  1 sibling, 1 reply; 14+ messages in thread
From: Eric Biggers @ 2019-06-18 18:18 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: Ard Biesheuvel, netdev,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, Jun 18, 2019 at 02:53:05AM -0700, Eric Dumazet wrote:
> On Tue, Jun 18, 2019 at 2:41 AM Ard Biesheuvel
> <ard.biesheuvel@linaro.org> wrote:
> >
> > On Tue, 18 Jun 2019 at 11:39, Eric Dumazet <edumazet@google.com> wrote:
> > >
> > > On Tue, Jun 18, 2019 at 2:32 AM Ard Biesheuvel
> > > <ard.biesheuvel@linaro.org> wrote:
> > > >
> > > > Some changes to the TCP fastopen code to make it more robust
> > > > against future changes in the choice of key/cookie size, etc.
> > > >
> > > > - Instead of keeping the SipHash key in an untyped u8[] buffer
> > > >   and casting it to the right type upon use, use the correct
> > > >   siphash_key_t type directly. This ensures that the key will
> > > >   appear at the correct alignment if we ever change the way
> > > >   these data structures are allocated. (Currently, they are
> > > >   only allocated via kmalloc so they always appear at the
> > > >   correct alignment)
> > > >
> > > > - Use DIV_ROUND_UP when sizing the u64[] array to hold the
> > > >   cookie, so it is always of sufficient size, even when
> > > >   TCP_FASTOPEN_COOKIE_MAX is no longer a multiple of 8.
> > > >
> > > > - Add a key length check to tcp_fastopen_reset_cipher(). No
> > > >   callers exist currently that fail this check (they all pass
> > > >   compile constant values that equal TCP_FASTOPEN_KEY_LENGTH),
> > > >   but future changes might create problems, e.g., by leaving part
> > > >   of the key uninitialized, or overflowing the key buffers.
> > > >
> > > > Note that none of these are functional changes wrt the current
> > > > state of the code.
> > > >
> > > ...
> > >
> > > > -       memcpy(ctx->key[0], primary_key, len);
> > > > +       if (unlikely(len != TCP_FASTOPEN_KEY_LENGTH)) {
> > > > +               pr_err("TCP: TFO key length %u invalid\n", len);
> > > > +               err = -EINVAL;
> > > > +               goto out;
> > > > +       }
> > >
> > >
> > > Why a pr_err() is there ?
> > >
> > > Can unpriv users flood the syslog ?
> >
> > They can if they could do so before: there was a call to
> > crypto_cipher_setkey() in the original pre-SipHash code which would
> > also result in a pr_err() on an invalid key length. That call got
> > removed along with the AES cipher handling, and this basically
> > reinstates it, as suggested by EricB.
> 
> This tcp_fastopen_reset_cipher() function is internal to TCP stack, all callers
> always pass the correct length.
> 
> We could add checks all over the place, and end up having a TCP stack
> full of defensive
> checks and 10,000 additional lines of code :/
> 
> I would prefer not reinstating this.

The length parameter makes no sense if it's not checked, though.  Either it
should exist and be checked, or it should be removed and the length should be
implicitly TCP_FASTOPEN_KEY_LENGTH.

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/2] net: fastopen: make key handling more robust against future changes
  2019-06-18 18:18         ` Eric Biggers
@ 2019-06-18 18:19           ` Eric Dumazet
  0 siblings, 0 replies; 14+ messages in thread
From: Eric Dumazet @ 2019-06-18 18:19 UTC (permalink / raw)
  To: Eric Biggers
  Cc: Ard Biesheuvel, netdev,
	open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
	David Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, Jason Baron,
	Christoph Paasch, David Laight, Yuchung Cheng

On Tue, Jun 18, 2019 at 11:18 AM Eric Biggers <ebiggers@kernel.org> wrote:

> The length parameter makes no sense if it's not checked, though.  Either it
> should exist and be checked, or it should be removed and the length should be
> implicitly TCP_FASTOPEN_KEY_LENGTH.

Sure, please send a patch.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie
  2019-06-18  9:32 ` [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie Ard Biesheuvel
@ 2019-06-18 18:22   ` Eric Biggers
  2019-06-18 18:40     ` Ard Biesheuvel
  0 siblings, 1 reply; 14+ messages in thread
From: Eric Biggers @ 2019-06-18 18:22 UTC (permalink / raw)
  To: Ard Biesheuvel
  Cc: netdev, linux-crypto, herbert, edumazet, davem, kuznet, yoshfuji,
	jbaron, cpaasch, David.Laight, ycheng

On Tue, Jun 18, 2019 at 11:32:07AM +0200, Ard Biesheuvel wrote:
> Use an explicit little endian representation of the fastopen
> cookie, so that the value no longer depends on the endianness
> of the system. This fixes a theoretical issue only, since
> fastopen keys are unlikely to be shared across load balancing
> server farms that are mixed in endiannes, but it might pop up
> in validation/selftests as well, so let's just settle on little
> endian across the board.
> 
> Note that this change only affects big endian systems.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  include/linux/tcp.h     |  2 +-
>  net/ipv4/tcp_fastopen.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 9 deletions(-)
> 

What about the TCP_FASTOPEN_KEY option for setsockopt and getsockopt?  Those
APIs treat the key as an array of bytes (let's say it's little endian), so
doesn't it need to be converted to/from the CPU endianness of siphash_key_t?

- Eric

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie
  2019-06-18 18:22   ` Eric Biggers
@ 2019-06-18 18:40     ` Ard Biesheuvel
  2019-06-18 22:40       ` David Miller
  0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2019-06-18 18:40 UTC (permalink / raw)
  To: Eric Biggers
  Cc: <netdev, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
	Herbert Xu, Eric Dumazet, David S. Miller, Alexey Kuznetsov,
	Hideaki YOSHIFUJI, Jason Baron, Christoph Paasch, David Laight,
	Yuchung Cheng

On Tue, 18 Jun 2019 at 20:22, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Tue, Jun 18, 2019 at 11:32:07AM +0200, Ard Biesheuvel wrote:
> > Use an explicit little endian representation of the fastopen
> > cookie, so that the value no longer depends on the endianness
> > of the system. This fixes a theoretical issue only, since
> > fastopen keys are unlikely to be shared across load balancing
> > server farms that are mixed in endiannes, but it might pop up
> > in validation/selftests as well, so let's just settle on little
> > endian across the board.
> >
> > Note that this change only affects big endian systems.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  include/linux/tcp.h     |  2 +-
> >  net/ipv4/tcp_fastopen.c | 16 ++++++++--------
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
>
> What about the TCP_FASTOPEN_KEY option for setsockopt and getsockopt?  Those
> APIs treat the key as an array of bytes (let's say it's little endian), so
> doesn't it need to be converted to/from the CPU endianness of siphash_key_t?
>

Yeah, good point.

Can we first agree on whether we care about this or not? If so, i can spin a v2.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie
  2019-06-18 18:40     ` Ard Biesheuvel
@ 2019-06-18 22:40       ` David Miller
  0 siblings, 0 replies; 14+ messages in thread
From: David Miller @ 2019-06-18 22:40 UTC (permalink / raw)
  To: ard.biesheuvel
  Cc: ebiggers, netdev, linux-crypto, herbert, edumazet, kuznet,
	yoshfuji, jbaron, cpaasch, David.Laight, ycheng

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Tue, 18 Jun 2019 20:40:18 +0200

> Can we first agree on whether we care about this or not? If so, i
> can spin a v2.

Well, how can it possibly work otherwise in deployment scenerios involving
both big and little endian hosts?

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-18  9:32 [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Ard Biesheuvel
2019-06-18  9:32 ` [PATCH 1/2] net: fastopen: make key handling more robust against future changes Ard Biesheuvel
2019-06-18  9:39   ` Eric Dumazet
2019-06-18  9:41     ` Ard Biesheuvel
2019-06-18  9:53       ` Eric Dumazet
2019-06-18 10:02         ` Ard Biesheuvel
2019-06-18 18:18         ` Eric Biggers
2019-06-18 18:19           ` Eric Dumazet
2019-06-18  9:32 ` [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie Ard Biesheuvel
2019-06-18 18:22   ` Eric Biggers
2019-06-18 18:40     ` Ard Biesheuvel
2019-06-18 22:40       ` David Miller
2019-06-18  9:37 ` [PATCH 0/2] net: fastopen: follow-up tweaks for SipHash switch Eric Dumazet
2019-06-18  9:38   ` Ard Biesheuvel

Netdev Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/netdev/0 netdev/git/0.git
	git clone --mirror https://lore.kernel.org/netdev/1 netdev/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 netdev netdev/ https://lore.kernel.org/netdev \
		netdev@vger.kernel.org netdev@archiver.kernel.org
	public-inbox-index netdev


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.netdev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox