Messages in this thread | | | From | Leonard Crestez <> | Subject | Re: [RFCv3 07/15] tcp: authopt: Hook into tcp core | Date | Wed, 25 Aug 2021 19:32:44 +0300 |
| |
On 25.08.2021 01:59, Eric Dumazet wrote: > On 8/24/21 2:34 PM, Leonard Crestez wrote: >> The tcp_authopt features exposes a minimal interface to the rest of the >> TCP stack. Only a few functions are exposed and if the feature is >> disabled they return neutral values, avoiding ifdefs in the rest of the >> code. >> >> Add calls into tcp authopt from send, receive and accept code. >> >> Signed-off-by: Leonard Crestez <cdleonard@gmail.com> >> --- >> include/net/tcp_authopt.h | 56 +++++++++ >> net/ipv4/tcp_authopt.c | 246 ++++++++++++++++++++++++++++++++++++++ >> net/ipv4/tcp_input.c | 17 +++ >> net/ipv4/tcp_ipv4.c | 3 + >> net/ipv4/tcp_minisocks.c | 2 + >> net/ipv4/tcp_output.c | 74 +++++++++++- >> net/ipv6/tcp_ipv6.c | 4 + >> 7 files changed, 401 insertions(+), 1 deletion(-) >> >> diff --git a/include/net/tcp_authopt.h b/include/net/tcp_authopt.h >> index c9ee2059b442..61db268f36f8 100644 >> --- a/include/net/tcp_authopt.h >> +++ b/include/net/tcp_authopt.h >> @@ -21,10 +21,11 @@ struct tcp_authopt_key_info { >> /* Wire identifiers */ >> u8 send_id, recv_id; >> u8 alg_id; >> u8 keylen; >> u8 key[TCP_AUTHOPT_MAXKEYLEN]; >> + u8 maclen; > > I do not see maclen being enforced to 12, or a multiple of 4 ?
For both current algorithms the maclen value is 12. I just implemented RFC5926, there is no way to control this from userspace.
> This means that later [2], tcp_authopt_hash() will leave up to 3 > unitialized bytes in the TCP options, sent to the wire. > > This is a security issue, since we will leak kernel memory.
Filling the remainder with zeroes does make sense, or at least WARN_ON(maclen != 4) so that it's obvious to anyone who attempts to extend the algorithms.
>> +struct tcp_authopt_key_info *tcp_authopt_lookup_send(struct tcp_authopt_info *info, >> + const struct sock *addr_sk, >> + int send_id) >> +{ >> + struct tcp_authopt_key_info *result = NULL; >> + struct tcp_authopt_key_info *key; >> + >> + hlist_for_each_entry_rcu(key, &info->head, node, 0) { >> + if (send_id >= 0 && key->send_id != send_id) >> + continue; >> + if (key->flags & TCP_AUTHOPT_KEY_ADDR_BIND) { >> + if (addr_sk->sk_family == AF_INET) { >> + struct sockaddr_in *key_addr = (struct sockaddr_in *)&key->addr; >> + const struct in_addr *daddr = >> + (const struct in_addr *)&addr_sk->sk_daddr; > > Why a cast is needed ? sk_daddr is a __be32, no need to cast it to in_addr >> + >> + if (WARN_ON(key_addr->sin_family != AF_INET)) > > Why a WARN_ON() is used ? If we expect this to trigger, then at minimumum WARN_ON_ONCE() please. > >> + continue; >> + if (memcmp(daddr, &key_addr->sin_addr, sizeof(*daddr))) >> + continue; > > Using memcmp() to compare two __be32 is overkill. > >> + } >> + if (addr_sk->sk_family == AF_INET6) { >> + struct sockaddr_in6 *key_addr = (struct sockaddr_in6 *)&key->addr; >> + const struct in6_addr *daddr = &addr_sk->sk_v6_daddr; > > Not sure why a variable is used, you need it once. > >> + >> + if (WARN_ON(key_addr->sin6_family != AF_INET6)) >> + continue; >> + if (memcmp(daddr, &key_addr->sin6_addr, sizeof(*daddr))) > > ipv6_addr_equal() should be faster.
OK, I will replace the comparisons.
Checking address family is mostly paranoia on my part, I don't know if a real scenario exists for AF mismatch. Still need to check ipv4-mapped ipv6 addresses, not sure if those can receive ipv4 skbs on an ipv6 socket.
>> +struct tcp_authopt_key_info *tcp_authopt_select_key(const struct sock *sk, >> + const struct sock *addr_sk, >> + u8 *rnextkeyid) >> +{ >> + struct tcp_authopt_info *info; >> + >> + info = rcu_dereference(tcp_sk(sk)->authopt_info); > > distro kernels will have CONFIG_TCP_AUTHOPT set, meaning > that we will add a cache line miss for every incoming TCP packet > even on hosts not using any RFC5925 TCP flow. > > For TCP MD5 we are using a static key, to avoid this extra cost.
OK, will add a static_key.
The check for "does socket have tcp_authopt" also belongs in an inline wrapper, similar to inbound check
>> +int __tcp_authopt_openreq(struct sock *newsk, const struct sock *oldsk, struct request_sock *req) >> +{ >> + struct tcp_authopt_info *old_info; >> + struct tcp_authopt_info *new_info; >> + int err; >> + >> + old_info = rcu_dereference(tcp_sk(oldsk)->authopt_info); >> + if (!old_info) >> + return 0; >> + >> + new_info = kmalloc(sizeof(*new_info), GFP_ATOMIC | __GFP_ZERO); > > kzalloc() is your friend. (same remark for your other patches, where you are using __GFP_ZERO) > Also see additional comment [1]
OK > >> + if (!new_info) >> + return -ENOMEM; >> + >> + sk_nocaps_add(newsk, NETIF_F_GSO_MASK); >> + new_info->src_isn = tcp_rsk(req)->snt_isn; >> + new_info->dst_isn = tcp_rsk(req)->rcv_isn; >> + INIT_HLIST_HEAD(&new_info->head); >> + err = tcp_authopt_clone_keys(newsk, oldsk, new_info, old_info); >> + if (err) { >> + __tcp_authopt_info_free(newsk, new_info); > > Are we leaving in place old value of newsk->authopt_info ? > If this is copied from the listener, I think you need > to add a tcp_sk(newsk)->authopt_info = NULL; > before the kzalloc() call done above.
Yes, authopt_info should be set to NULL on error because keeping the listen socket's value is wrong and dangerous (double free).
Leaving authopt_info NULL or malloc failure is still possible dangerous because it means all keys are ignored and accepted. Not clear how we could cause tcp_create_openreq_child to fail instead.
This is a problem in a few other parts: if cryptography fails the outbound MAC is filled with zeros because there's not obvious way to make TX fail at that point.
>> + err = __tcp_authopt_calc_mac(sk, skb, key, false, macbuf); >> + if (err) { >> + /* If mac calculation fails and caller doesn't handle the error >> + * try to make it obvious inside the packet. >> + */ >> + memset(hash_location, 0, key->maclen); >> + return err; >> + } >> + memcpy(hash_location, macbuf, key->maclen); > > > [2] > This is the place were we do not make sure to clear the padding bytes > (if key->maclen is not a multiple of 4)
Yes. It might make sense to fix in caller because it's the caller which decides to align options.
| |