Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca <sd@queasysnail.net>]

2023-01-18, 23:10:18 +0000, Vadim Fedorenko wrote:
> On 17.01.2023 13:45, Sabrina Dubroca wrote:
> > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >   		alt_crypto_info = &ctx->crypto_send.info;
> >   	}
> > -	/* Currently we don't support set crypto info more than one time */
> > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > -		return -EBUSY;
> > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > +		/* Currently we only support setting crypto info more
> > +		 * than one time for TLS 1.3
> > +		 */
> > +		if (crypto_info->version != TLS_1_3_VERSION)
> > +			return -EBUSY;
> > +
> > +		update = true;
> > +		old_crypto_info = crypto_info;
> > +		crypto_info = &tmp.info;
> > +	}
> >   	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
> >   	if (rc) {
> > @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >   		goto err_crypto_info;
> >   	}
> > +	if (update) {
> > +		/* Ensure that TLS version and ciphers are not modified */
> > +		if (crypto_info->version != old_crypto_info->version ||
> > +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> > +			rc = -EINVAL;
> > +			goto err_crypto_info;
> > +		}
> > +	}
> > +
> 
> looks like these checks can be moved up to TLS_CRYPTO_INFO_READY scope and
> there will be no need for extra variables.

I don't see how to do that cleanly. I'd have to duplicate the
copy_from_sockptr, which IMHO looks a lot worse. Is there another way?

> >   	/* Ensure that TLS version and ciphers are same in both directions */
> >   	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
> >   		if (alt_crypto_info->version != crypto_info->version ||

[...]
> > @@ -2794,12 +2852,14 @@ int tls_set_sw_offload(struct sock *sk, int tx)
> >   	kfree(cctx->iv);
> >   	cctx->iv = NULL;
> >   free_priv:
> > -	if (tx) {
> > -		kfree(ctx->priv_ctx_tx);
> > -		ctx->priv_ctx_tx = NULL;
> > -	} else {
> > -		kfree(ctx->priv_ctx_rx);
> > -		ctx->priv_ctx_rx = NULL;
> > +	if (!new_crypto_info) {
> > +		if (tx) {
> > +			kfree(ctx->priv_ctx_tx);
> > +			ctx->priv_ctx_tx = NULL;
> > +		} else {
> > +			kfree(ctx->priv_ctx_rx);
> > +			ctx->priv_ctx_rx = NULL;
> > +		}
> >   	}
> >   out:
> >   	return rc;
> 
> I think we can avoid extra parameter and extra level of if{} constructions
> by checking if iv and rec_seq is already allocated and adjust init part the
> same way. I don't think we have to have separate error path because in case
> of any error during rekey procedure the connection becomes useless and
> application should indicate error to the other end. The code copies new
> crypto info to the current storage, so it assumes that all fields a properly
> filled and that means that this copy can be done earlier and use the same
> code path as first init code.

Rekey could fail because of memory allocation failures during
crypto_aead_setkey. Userspace could choose to retry the key update,
and we shouldn't necessarily kill off the connection in that case.

I think we need to keep the init/update distinction in the error paths
for tls_set_sw_offload and do_tls_setsockopt_conf, otherwise we clear
the crypto_info from the context and a new attempt to do the rekey
will run through the full init path instead of the rekey path.

We could set crypto_info in tls_context before calling
tls_set_sw_offload, but do_tls_setsockopt_conf would still have some
differences since we need to validate that the version/cipher hasn't
changed. I'll give that a try and see how much that improves
things. It should reduce the churn a bit.

Thanks

-- 
Sabrina