Hi Leonard,
On 8/24/21 10:34 PM, Leonard Crestez wrote: [..]
--- /dev/null +++ b/include/net/tcp_authopt.h @@ -0,0 +1,65 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef _LINUX_TCP_AUTHOPT_H +#define _LINUX_TCP_AUTHOPT_H
+#include <uapi/linux/tcp.h>
+/**
- struct tcp_authopt_key_info - Representation of a Master Key Tuple as per RFC5925
- Key structure lifetime is only protected by RCU so readers needs to hold a
- single rcu_read_lock until they're done with the key.
- */
+struct tcp_authopt_key_info {
- struct hlist_node node;
- struct rcu_head rcu;
- /* Local identifier */
- u32 local_id;
It's unused now, can be removed.
[..]
+/**
- enum tcp_authopt_key_flag - flags for `tcp_authopt.flags`
- @TCP_AUTHOPT_KEY_DEL: Delete the key by local_id and ignore all other fields.
^ By send_id and recv_id. Also, tcp_authopt_key_match_exact() seems to check TCP_AUTHOPT_KEY_ADDR_BIND. I wounder if that makes sense to relax it in case of TCP_AUTHOPT_KEY_DEL to match only send_id/recv_id if addr isn't specified (no hard feelings about it, though).
[..]
+#ifdef CONFIG_TCP_AUTHOPT
- case TCP_AUTHOPT: {
struct tcp_authopt info;if (get_user(len, optlen))return -EFAULT;lock_sock(sk);tcp_get_authopt_val(sk, &info);release_sock(sk);len = min_t(unsigned int, len, sizeof(info));if (put_user(len, optlen))return -EFAULT;if (copy_to_user(optval, &info, len))return -EFAULT;return 0;
Failed tcp_get_authopt_val() lookup in: : if (!info) : return -EINVAL;
will leak uninitialized kernel memory from stack. ASLR guys defeated.
[..]
+#define TCP_AUTHOPT_KNOWN_FLAGS ( \
- TCP_AUTHOPT_FLAG_REJECT_UNEXPECTED)
+int tcp_set_authopt(struct sock *sk, sockptr_t optval, unsigned int optlen) +{
- struct tcp_authopt opt;
- struct tcp_authopt_info *info;
- sock_owned_by_me(sk);
- /* If userspace optlen is too short fill the rest with zeros */
- if (optlen > sizeof(opt))
return -EINVAL;
More like : if (unlikely(len > sizeof(opt))) { : err = check_zeroed_user(optval + sizeof(opt), : len - sizeof(opt)); : if (err < 1) : return err == 0 ? -EINVAL : err; : len = sizeof(opt); : if (put_user(len, optlen)) : return -EFAULT; : }
- memset(&opt, 0, sizeof(opt));
- if (copy_from_sockptr(&opt, optval, optlen))
return -EFAULT;- if (opt.flags & ~TCP_AUTHOPT_KNOWN_FLAGS)
return -EINVAL;- info = __tcp_authopt_info_get_or_create(sk);
- if (IS_ERR(info))
return PTR_ERR(info);- info->flags = opt.flags & TCP_AUTHOPT_KNOWN_FLAGS;
- return 0;
+}
[..]
+int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen) +{
- struct tcp_authopt_key opt;
- struct tcp_authopt_info *info;
- struct tcp_authopt_key_info *key_info;
- sock_owned_by_me(sk);
- /* If userspace optlen is too short fill the rest with zeros */
- if (optlen > sizeof(opt))
return -EINVAL;
Ditto
- memset(&opt, 0, sizeof(opt));
- if (copy_from_sockptr(&opt, optval, optlen))
return -EFAULT;- if (opt.flags & ~TCP_AUTHOPT_KEY_KNOWN_FLAGS)
return -EINVAL;- if (opt.keylen > TCP_AUTHOPT_MAXKEYLEN)
return -EINVAL;- /* Delete is a special case: */
- if (opt.flags & TCP_AUTHOPT_KEY_DEL) {
info = rcu_dereference_check(tcp_sk(sk)->authopt_info, lockdep_sock_is_held(sk));if (!info)return -ENOENT;key_info = tcp_authopt_key_lookup_exact(sk, info, &opt);if (!key_info)return -ENOENT;tcp_authopt_key_del(sk, info, key_info);
Doesn't seem to be safe together with tcp_authopt_select_key(). A key can be in use at this moment - you have to add checks for it.
return 0;- }
- /* check key family */
- if (opt.flags & TCP_AUTHOPT_KEY_ADDR_BIND) {
if (sk->sk_family != opt.addr.ss_family)return -EINVAL;- }
- /* Initialize tcp_authopt_info if not already set */
- info = __tcp_authopt_info_get_or_create(sk);
- if (IS_ERR(info))
return PTR_ERR(info);- /* If an old key exists with exact ID then remove and replace.
* RCU-protected readers might observe both and pick any.*/- key_info = tcp_authopt_key_lookup_exact(sk, info, &opt);
- if (key_info)
tcp_authopt_key_del(sk, info, key_info);- key_info = sock_kmalloc(sk, sizeof(*key_info), GFP_KERNEL | __GFP_ZERO);
- if (!key_info)
return -ENOMEM;
So, you may end up without any key. Also, replacing a key is not at all safe: you may receive old segments which you in turn will discard and reset the connection.
I think the limitation RFC puts on removing keys in use and replacing existing keys are actually reasonable. Probably, it'd be better to enforce "key in use => desired key is different (or key_outdated flag) => key not in use => key may be removed" life-cycle of MKT.
Thanks, Dmitry