Re: [PATCH RFC net-next 3/5] tls: update the RX key when a KeyUpdate is received

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tls: memory leak of decrypted skb when tls_keyupdate_rx() fails
- [Medium] tls: spurious saved_data_ready() call during in-kernel RX rekey
--

commit 52ccace8026bcb8f3e4f14ffabc2a54f8cb3955e
Author: Martin Belanger <martin.belanger@dell.com>

tls: update the RX key when a KeyUpdate is received

This commit updates the kTLS receive path so sockets with a registered RX
traffic secret automatically derive and install the next key upon decrypting
a TLS 1.3 KeyUpdate record. The consumed KeyUpdate record is not passed to
the caller, and invalid or excessive KeyUpdate messages abort the connection.

> diff --git a/net/tls/tls_keyupdate.c b/net/tls/tls_keyupdate.c
> new file mode 100644
> index 0000000000000..543bf57599c7b
> --- /dev/null
> +++ b/net/tls/tls_keyupdate.c

[ ... ]

> +int tls_keyupdate_rekey(struct sock *sk, int tx)
> +{
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	struct tls_traffic_secret *secret;
> +	const struct tls_cipher_desc *cipher_desc;
> +	u8 iv[TLS_MAX_SALT_SIZE + TLS_MAX_IV_SIZE];
> +	u8 next[TLS_MAX_TRAFFIC_SECRET_SIZE];
> +	union tls_crypto_context tmp = {};
> +	struct tls_crypto_info *info = &tmp.info;
> +	int rc;
> +
> +	secret = tx ? &ctx->tx_secret : &ctx->rx_secret;
> +	cipher_desc = get_cipher_desc(ctx->prot_info.cipher_type);
> +	if (!cipher_desc || !secret->len ||
> +	    cipher_desc->salt + cipher_desc->iv > sizeof(iv))
> +		return -EINVAL;
> +
> +	/* version and cipher_type; the rest of tmp stays zero */
> +	*info = tx ? ctx->crypto_send.info : ctx->crypto_recv.info;
> +
> +	rc = tls_hkdf_expand_label(secret->data, secret->len, "traffic upd",
> +				   next, secret->len);
> +	if (rc)
> +		goto out;
> +
> +	rc = tls_hkdf_expand_label(next, secret->len, "key",
> +				   crypto_info_key(info, cipher_desc),
> +				   cipher_desc->key);
> +	if (rc)
> +		goto out;
> +
> +	rc = tls_hkdf_expand_label(next, secret->len, "iv", iv,
> +				   cipher_desc->salt + cipher_desc->iv);
> +	if (rc)
> +		goto out;
> +
> +	memcpy(crypto_info_salt(info, cipher_desc), iv, cipher_desc->salt);
> +	memcpy(crypto_info_iv(info, cipher_desc), iv + cipher_desc->salt,
> +	       cipher_desc->iv);
> +
> +	rc = tls_set_sw_offload(sk, tx, info);
> +	if (rc)
> +		goto out;

[Severity: Medium]
When tls_keyupdate_rekey() is called on the RX path (tx == 0) via:

  tls_rx_one_record()
    -> tls_check_pending_rekey()
      -> tls_keyupdate_rx()
        -> tls_keyupdate_rekey()
          -> tls_set_sw_offload()
            -> tls_finish_key_update()

tls_finish_key_update() unconditionally invokes ctx->saved_data_ready(sk):

  static void tls_finish_key_update(struct sock *sk, struct tls_context *tls_ctx)
  {
          struct tls_sw_context_rx *ctx = tls_ctx->priv_ctx_rx;

          WRITE_ONCE(ctx->key_update_pending, false);
          /* wake-up pre-existing poll() */
          ctx->saved_data_ready(sk);
  }

In this in-kernel RX rekey path, ctx->key_update_pending was never set to
true, and tls_rx_rec_done(ctx) has not been called yet by the caller
(tls_sw_recvmsg(), tls_sw_splice_read(), or tls_sw_read_sock()), so
ctx->strp.msg_ready still refers to the unconsumed KeyUpdate record.

Won't calling ctx->saved_data_ready(sk) here cause concurrent pollers running
tls_sk_poll() to observe tls_strp_msg_ready(ctx) == true and falsely report
EPOLLIN | EPOLLRDNORM when no application data record follows, as well as
schedule spurious receive work in kernel consumers like nvme-tcp and sunrpc
that commit 22f8bf8808dc ("tls: Suppress spurious saved_data_ready on all
receive paths") aimed to avoid?

[ ... ]

> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 312e51270f293..a314fa638dc36 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -1551,15 +1551,26 @@ static int tls_check_pending_rekey(struct sock *sk, struct tls_context *ctx,
>  	if (hs_type == TLS_HANDSHAKE_KEYUPDATE) {
>  		struct tls_sw_context_rx *rx_ctx = ctx->priv_ctx_rx;
>  
> -		WRITE_ONCE(rx_ctx->key_update_pending, true);
>  		TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXREKEYRECEIVED);
> +
> +		if (ctx->rx_secret.len) {
> +			err = tls_keyupdate_rx(sk, skb);
> +			if (err) {
> +				tls_err_abort(sk, err);
> +				return err;
> +			}
> +			return 1;
> +		}

[Severity: High]
Does this error path leak the decrypted skb (darg->skb)?

In tls_rx_one_record(), tls_decrypt_sw() (or tls_decrypt_device()) has
already allocated a standalone cleartext skb in darg->skb for the
TLS_RECORD_TYPE_HANDSHAKE record before calling tls_check_pending_rekey().

When tls_keyupdate_rx() returns a negative errno (for example, due to a
malformed KeyUpdate message or exceeding TLS_KEYUPDATE_RX_MAX_PER_SEC),
tls_check_pending_rekey() returns that negative error to tls_rx_one_record()
without freeing skb.

All three callers of tls_rx_one_record() only call consume_skb(darg.skb) when
err >= 0, and jump straight to their exit labels when err < 0:

  err = tls_rx_one_record(sk, msg, &darg);
  if (err < 0)
          goto recv_end; /* or splice_read_end / read_sock_end */
  if (err > 0) {
          tls_rx_rec_done(ctx);
          consume_skb(darg.skb);
          continue;
  }

Should skb be freed via consume_skb(skb) before returning a negative error
from tls_check_pending_rekey()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025-keyupdate-rfc-v1-0-60b7756eccbd@gmail.com?part=3