Re: [PATCH net-next v5 4/6] 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: FORTIFY_SOURCE field-spanning write warning in tls_keyupdate_rekey()
--

commit 0485598776b0569167bc509b38a622743e5ca7ce
Author: Martin Belanger <martin.belanger@dell.com>

tls: update the RX key when a KeyUpdate is received

This commit updates kTLS to derive and install the next RX key automatically
when a KeyUpdate handshake message is received on a socket with a registered
RX traffic secret. It also validates the KeyUpdate message format and rate
limits incoming KeyUpdate messages to at most 8 per second.

> diff --git a/net/tls/tls_keyupdate.c b/net/tls/tls_keyupdate.c
> new file mode 100644
> index 0000000000000..b9e1ecfc59c47
> --- /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 */
> +	memset(&tmp, 0, sizeof(tmp));
> +	*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);

[Severity: High]
Won't these memcpy() calls trigger a CONFIG_FORTIFY_SOURCE field-spanning
write warning when tls_check_pending_rekey() -> tls_keyupdate_rx() ->
tls_keyupdate_rekey() runs on a socket with a registered traffic secret?

In tls_keyupdate_rekey(), info is initialized as &tmp.info, which points to
the 4-byte struct tls_crypto_info subobject inside union tls_crypto_context
tmp.

The inlined helpers crypto_info_salt() and crypto_info_iv() offset from info
past the end of that 4-byte member:

net/tls/tls.h:
static inline char *crypto_info_iv(struct tls_crypto_info *crypto_info,
				   const struct tls_cipher_desc *cipher_desc)
{
	return (char *)crypto_info + cipher_desc->iv_offset;
}

static inline char *crypto_info_salt(struct tls_crypto_info *crypto_info,
				     const struct tls_cipher_desc *cipher_desc)
{
	return (char *)crypto_info + cipher_desc->salt_offset;
}

Because the destination pointer is derived from the union member &tmp.info
rather than the enclosing union tmp, __member_size() evaluates to 0 while
__struct_size() evaluates to the remaining space in tmp, causing fortified
memcpy() to warn on a field-spanning write.

This is the same union subobject bounds issue that commit c43c1c24f3cd ("tls:
avoid field-spanning write when copying crypto info") fixed in
do_tls_setsockopt_conf(). Could the destination offset be computed from &tmp
(or could crypto_info_salt() and crypto_info_iv() take union
tls_crypto_context *) to avoid the FORTIFY_SOURCE warning?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-keyupdate-rfc-v5-0-de1ab90a88d5@gmail.com?part=4