On 11/5/21 3:22 AM, Dmitry Safonov wrote:
Hi Leonard,
On 11/1/21 16:34, Leonard Crestez wrote: [..]
+struct tcp_authopt_key {
- /** @flags: Combination of &enum tcp_authopt_key_flag */
- __u32 flags;
- /** @send_id: keyid value for send */
- __u8 send_id;
- /** @recv_id: keyid value for receive */
- __u8 recv_id;
- /** @alg: One of &enum tcp_authopt_alg */
- __u8 alg;
- /** @keylen: Length of the key buffer */
- __u8 keylen;
- /** @key: Secret key */
- __u8 key[TCP_AUTHOPT_MAXKEYLEN];
- /**
* @addr: Key is only valid for this address** Ignored unless TCP_AUTHOPT_KEY_ADDR_BIND flag is set*/- struct __kernel_sockaddr_storage addr;
+};
[..]
+/* Free key nicely, for living sockets */ +static void tcp_authopt_key_del(struct sock *sk,
struct tcp_authopt_info *info,struct tcp_authopt_key_info *key)+{
- sock_owned_by_me(sk);
- hlist_del_rcu(&key->node);
- atomic_sub(sizeof(*key), &sk->sk_omem_alloc);
- kfree_rcu(key, rcu);
+}
[..]
+#define TCP_AUTHOPT_KEY_KNOWN_FLAGS ( \
- TCP_AUTHOPT_KEY_DEL | \
- TCP_AUTHOPT_KEY_EXCLUDE_OPTS | \
- TCP_AUTHOPT_KEY_ADDR_BIND)
+int tcp_set_authopt_key(struct sock *sk, sockptr_t optval, unsigned int optlen) +{
[..]
- /* 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);return 0;I remember we discussed it in RFC, that removing a key that's currently in use may result in random MKT to be used.
I think, it's possible to make this API a bit more predictable if:
- DEL command fails to remove a key that is current/receive_next;
- opt.flags has CURR/NEXT flag that has corresponding `u8 current_key`
and `u8 receive_next` values. As socket lock is held - that makes current_key/receive_next change atomic with deletion of an existing key that might have been in use.
In result user may remove a key that's not in use or has to set new current/next. Which avoids the issue with random MKT being used to sign segments.
The MKT used to sign segments is already essentially random unless the user makes a deliberate choice. This is what happens if you add two keys an call connect(). But why is this a problem?
Applications which want to deliberately control the send key can do so with TCP_AUTHOPT_FLAG_LOCK_KEYID. If that flag is not set then the key with send_id == recv_rnextkeyid is preffered as suggested by the RFC, or a random one on connect.
I think your suggestion would force additional complexity on all applications for no clear gain.
Key selection controls are only added much later in the series, this is also part of the effort to split the code into readable patches. See this patch:
https://lore.kernel.org/netdev/2dc569c0d60c80c26aafcaa201ba5b5ec53ce6bd.1635...
Removing a key while traffic is happening shouldn't cause failures in recv or send code; this takes some effort but is also required to prevent auth failures when a socket is closed and transitions to timewait. I attempted to ensure this by only doing rcu_dereference for tcp_authopt_info and tcp_authopt_key_info once per packet.
-- Regards, Leonard