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.1635784253.git.cdleonard@xxxxxxxxx/
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
