[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y8leRmSlHJK0zfCR@hog>
Date: Thu, 19 Jan 2023 16:14:14 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: Vadim Fedorenko <vfedorenko@...ek.ru>
Cc: Linux Kernel Network Developers <netdev@...r.kernel.org>,
Frantisek Krenzelok <fkrenzel@...hat.com>
Subject: Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3
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
Powered by blists - more mailing lists