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
