Re: [PATCH v2 01/25] tcp: authopt: Initial support and key management

[Leonard Crestez <cdleonard@gmail.com>]

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@gmail.com/

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