ACK/Cmnt: [PATCH 1/1][SRU][FOCAL][GROOVY] tcp: correct read of TFO keys on big endian systems
Colin Ian King
colin.king at canonical.com
Wed Aug 12 07:19:44 UTC 2020
On 12/08/2020 07:57, Stefan Bader wrote:
> 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.
Thanks for the comments. I've updated the bug to reflect the impact of
this change.
>
> -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,
>>
>
>
>
More information about the kernel-team
mailing list