Re: [PATCH net-next v4 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 1 potential issue(s) to consider:
- [High] tls: uninitialized rec_seq in tls_keyupdate_rekey() union tmp
--

commit 9c6271bd7dbba966cda26fd41765d631c75ecd94
Author: Martin Belanger <martin.belanger@dell.com>

tls: update the RX key when a KeyUpdate is received

This commit updates kTLS RX processing so that when a socket with a
registered RX traffic secret receives a valid KeyUpdate message, the kernel
derives and installs the next RX key and consumes the KeyUpdate record
without passing it to the caller.

> diff --git a/net/tls/tls_keyupdate.c b/net/tls/tls_keyupdate.c
> new file mode 100644
> index 0000000000000..281c7b13eb82b
> --- /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);

[Severity: High]
Does initializing tmp with {} leave the rec_seq portion of union
tls_crypto_context uninitialized when tls_check_pending_rekey() ->
tls_keyupdate_rx() -> tls_keyupdate_rekey() runs?

In include/net/tls.h, the first member of union tls_crypto_context is the
4-byte struct tls_crypto_info:

include/net/tls.h:
union tls_crypto_context {
	struct tls_crypto_info info;
	union {
		struct tls12_crypto_info_aes_gcm_128 aes_gcm_128;
		struct tls12_crypto_info_aes_gcm_256 aes_gcm_256;
		...
	};
};

On compilers without -fzero-init-padding-bits=all (such as Clang or GCC <
15), initializing a union with {} only zero-initializes its first member (4
bytes) and leaves the remaining 52 bytes of union tail padding uninitialized
(or filled with 0xaa under CONFIG_INIT_STACK_ALL_PATTERN=y).

Since tls_keyupdate_rekey() only writes info, key, salt, and iv into tmp
before calling tls_set_sw_offload(sk, tx, info), won't tls_set_sw_offload()
copy uninitialized or pattern-filled stack bytes from crypto_info_rec_seq()
into cctx->rec_seq and ctx->crypto_recv / ctx->crypto_send, causing
subsequent records to fail AEAD decryption with -EBADMSG and exposing stack
bytes via getsockopt(SOL_TLS, TLS_RX / TLS_TX)?

Should tls_keyupdate_rekey() zero tmp with memset(&tmp, 0, sizeof(tmp))
instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-keyupdate-rfc-v4-0-f17419972b9f@gmail.com?part=3