ACK/Cmnt: [PATCH 1/1][SRU][FOCAL][GROOVY] tcp: correct read of TFO keys on big endian systems
Stefan Bader
stefan.bader at canonical.com
Wed Aug 12 06:57:40 UTC 2020
On 11.08.20 18:01, Colin King wrote:
> From: Jason Baron <jbaron at akamai.com>
>
> BugLink: https://bugs.launchpad.net/bugs/1869134
>
> When TFO keys are read back on big endian systems either via the global
> sysctl interface or via getsockopt() using TCP_FASTOPEN_KEY, the values
> don't match what was written.
>
> For example, on s390x:
>
> # echo "1-2-3-4" > /proc/sys/net/ipv4/tcp_fastopen_key
> # cat /proc/sys/net/ipv4/tcp_fastopen_key
> 02000000-01000000-04000000-03000000
>
> Instead of:
>
> # cat /proc/sys/net/ipv4/tcp_fastopen_key
> 00000001-00000002-00000003-00000004
>
> Fix this by converting to the correct endianness on read. This was
> reported by Colin Ian King when running the 'tcp_fastopen_backup_key' net
> selftest on s390x, which depends on the read value matching what was
> written. I've confirmed that the test now passes on big and little endian
> systems.
>
> Signed-off-by: Jason Baron <jbaron at akamai.com>
> Fixes: 438ac88009bc ("net: fastopen: robustness and endianness fixes for SipHash")
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Cc: Eric Dumazet <edumazet at google.com>
> Reported-and-tested-by: Colin Ian King <colin.king at canonical.com>
> Signed-off-by: David S. Miller <davem at davemloft.net>
> (cherry picked from commit f19008e676366c44e9241af57f331b6c6edf9552 linux-next)
> Signed-off-by: Colin Ian King <colin.king at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
> ---
As Thadeu already brought up this is rather hard to really say whether this is a
correct fix from glancing at it. So we trust here in testing and it being a more
common codepath to take.
Before this gets included, please go over the bug report justification and get
the end(ian)s right.
Also, maybe that could improve impact as well as regression potential, what
exactly is the user impact. Reading this up here and in the bug sound a bit
like the test fails because content of a sysctl read is wrong. Or is that stored
incorrectly and has impact on certain open operations? Could it be narrowed down
when fast open would be used. All a bit from the point of view of an admin or
user of a system. What would be curently observed or if something goes wrong.
-Stefan
> include/net/tcp.h | 2 ++
> net/ipv4/sysctl_net_ipv4.c | 16 ++++------------
> net/ipv4/tcp.c | 16 ++++------------
> net/ipv4/tcp_fastopen.c | 23 +++++++++++++++++++++++
> 4 files changed, 33 insertions(+), 24 deletions(-)
>
> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index dbf5c791a6eb..eab6c7510b5b 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -1672,6 +1672,8 @@ void tcp_fastopen_destroy_cipher(struct sock *sk);
> void tcp_fastopen_ctx_destroy(struct net *net);
> int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> void *primary_key, void *backup_key);
> +int tcp_fastopen_get_cipher(struct net *net, struct inet_connection_sock *icsk,
> + u64 *key);
> void tcp_fastopen_add_skb(struct sock *sk, struct sk_buff *skb);
> struct sock *tcp_try_fastopen(struct sock *sk, struct sk_buff *skb,
> struct request_sock *req,
> diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
> index 5653e3b011bf..54023a46db04 100644
> --- a/net/ipv4/sysctl_net_ipv4.c
> +++ b/net/ipv4/sysctl_net_ipv4.c
> @@ -301,24 +301,16 @@ static int proc_tcp_fastopen_key(struct ctl_table *table, int write,
> struct ctl_table tbl = { .maxlen = ((TCP_FASTOPEN_KEY_LENGTH *
> 2 * TCP_FASTOPEN_KEY_MAX) +
> (TCP_FASTOPEN_KEY_MAX * 5)) };
> - struct tcp_fastopen_context *ctx;
> - u32 user_key[TCP_FASTOPEN_KEY_MAX * 4];
> - __le32 key[TCP_FASTOPEN_KEY_MAX * 4];
> + u32 user_key[TCP_FASTOPEN_KEY_BUF_LENGTH / sizeof(u32)];
> + __le32 key[TCP_FASTOPEN_KEY_BUF_LENGTH / sizeof(__le32)];
> char *backup_data;
> - int ret, i = 0, off = 0, n_keys = 0;
> + int ret, i = 0, off = 0, n_keys;
>
> tbl.data = kmalloc(tbl.maxlen, GFP_KERNEL);
> if (!tbl.data)
> return -ENOMEM;
>
> - rcu_read_lock();
> - ctx = rcu_dereference(net->ipv4.tcp_fastopen_ctx);
> - if (ctx) {
> - n_keys = tcp_fastopen_context_len(ctx);
> - memcpy(&key[0], &ctx->key[0], TCP_FASTOPEN_KEY_LENGTH * n_keys);
> - }
> - rcu_read_unlock();
> -
> + n_keys = tcp_fastopen_get_cipher(net, NULL, (u64 *)key);
> if (!n_keys) {
> memset(&key[0], 0, TCP_FASTOPEN_KEY_LENGTH);
> n_keys = 1;
> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index c06d2bfd2ec4..31f3b858db81 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -3685,22 +3685,14 @@ static int do_tcp_getsockopt(struct sock *sk, int level,
> return 0;
>
> case TCP_FASTOPEN_KEY: {
> - __u8 key[TCP_FASTOPEN_KEY_BUF_LENGTH];
> - struct tcp_fastopen_context *ctx;
> - unsigned int key_len = 0;
> + u64 key[TCP_FASTOPEN_KEY_BUF_LENGTH / sizeof(u64)];
> + unsigned int key_len;
>
> if (get_user(len, optlen))
> return -EFAULT;
>
> - rcu_read_lock();
> - ctx = rcu_dereference(icsk->icsk_accept_queue.fastopenq.ctx);
> - if (ctx) {
> - key_len = tcp_fastopen_context_len(ctx) *
> - TCP_FASTOPEN_KEY_LENGTH;
> - memcpy(&key[0], &ctx->key[0], key_len);
> - }
> - rcu_read_unlock();
> -
> + key_len = tcp_fastopen_get_cipher(net, icsk, key) *
> + TCP_FASTOPEN_KEY_LENGTH;
> len = min_t(unsigned int, len, key_len);
> if (put_user(len, optlen))
> return -EFAULT;
> diff --git a/net/ipv4/tcp_fastopen.c b/net/ipv4/tcp_fastopen.c
> index 19ad9586c720..1bb85821f1e6 100644
> --- a/net/ipv4/tcp_fastopen.c
> +++ b/net/ipv4/tcp_fastopen.c
> @@ -108,6 +108,29 @@ int tcp_fastopen_reset_cipher(struct net *net, struct sock *sk,
> return err;
> }
>
> +int tcp_fastopen_get_cipher(struct net *net, struct inet_connection_sock *icsk,
> + u64 *key)
> +{
> + struct tcp_fastopen_context *ctx;
> + int n_keys = 0, i;
> +
> + rcu_read_lock();
> + if (icsk)
> + ctx = rcu_dereference(icsk->icsk_accept_queue.fastopenq.ctx);
> + else
> + ctx = rcu_dereference(net->ipv4.tcp_fastopen_ctx);
> + if (ctx) {
> + n_keys = tcp_fastopen_context_len(ctx);
> + for (i = 0; i < n_keys; i++) {
> + put_unaligned_le64(ctx->key[i].key[0], key + (i * 2));
> + put_unaligned_le64(ctx->key[i].key[1], key + (i * 2) + 1);
> + }
> + }
> + rcu_read_unlock();
> +
> + return n_keys;
> +}
> +
> static bool __tcp_fastopen_cookie_gen_cipher(struct request_sock *req,
> struct sk_buff *syn,
> const siphash_key_t *key,
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200812/a846e96f/attachment.sig>
More information about the kernel-team
mailing list