Re: [net-next,v10,11/15] quic: add crypto key derivation and installation

[Paolo Abeni <pabeni@redhat.com>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.

For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
---
quic: add crypto key derivation and installation

This patch introduces 'quic_crypto', a component responsible for QUIC
encryption key derivation and installation across the various key
levels: Initial, Handshake, 0-RTT (Early), and 1-RTT (Application).
It provides helpers to derive and install initial secrets, set traffic
secrets and install the corresponding keys, and perform key updates to
enable forward secrecy.

> diff --git a/net/quic/Makefile b/net/quic/Makefile
> index 9d8e18297911..58bb18f7926d 100644
> --- a/net/quic/Makefile
> +++ b/net/quic/Makefile
> @@ -6,4 +6,4 @@
>  obj-$(CONFIG_IP_QUIC) += quic.o
>
>  quic-y := common.o family.o protocol.o socket.o stream.o connid.o path.o \
> -	  cong.o pnspace.o
> +	  cong.o pnspace.o crypto.o
> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> new file mode 100644
> index 000000000000..a9de418e604e
> --- /dev/null
> +++ b/net/quic/crypto.c

[ ... ]

> +/* Initial Secrets. */
> +int quic_crypto_initial_keys_install(struct quic_crypto *crypto, struct quic_conn_id *conn_id,
> +				     u32 version, bool is_serv)
> +{
> +	u8 secret[TLS_CIPHER_AES_GCM_128_SECRET_SIZE];
> +	struct quic_data salt, s, k, l, dcid, z = {};
> +	struct quic_crypto_secret srt = {};
> +	char *tl, *rl, *sal;
> +	int err;
> +
> +	if (is_serv) {
> +		rl = "client in";
> +		tl = "server in";
> +	} else {
> +		tl = "client in";
> +		rl = "server in";
> +	}
> +	sal = QUIC_INITIAL_SALT_V1;
> +	if (version == QUIC_VERSION_V2)
> +		sal = QUIC_INITIAL_SALT_V2;
> +	quic_data(&salt, sal, QUIC_INITIAL_SALT_LEN);
> +	quic_data(&dcid, conn_id->data, conn_id->len);
> +	quic_data(&s, secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> +	err = quic_crypto_hkdf_extract(crypto->secret_tfm, &salt, &dcid, &s);

Is crypto->secret_tfm guaranteed to be initialized at this point?

Looking at the function flow, this line uses crypto->secret_tfm before
quic_crypto_set_secret() below gets a chance to initialize the cipher:

quic_crypto_initial_keys_install() {
	err = quic_crypto_hkdf_extract(crypto->secret_tfm, ...); /* line 490 */
	...
	err = quic_crypto_set_secret(crypto, &srt, version, 0);  /* line 497 */
}

If quic_crypto_set_cipher() hasn't been called on this struct quic_crypto
yet, crypto->secret_tfm is NULL.

This contrasts with quic_crypto_set_secret(), which has lazy initialization:

quic_crypto_set_secret() {
	if (!crypto->cipher) {
		err = quic_crypto_set_cipher(crypto, srt->type, flag);
		if (err)
			return err;
	}
	...
}

Without this initialization, quic_crypto_hkdf_extract() flows through
hkdf_extract() -> crypto_shash_setkey(NULL, ...), causing a NULL pointer
dereference.

Would it make sense to call quic_crypto_set_cipher() before using
secret_tfm, or to reorder the function so quic_crypto_set_secret() is
called first?

> +	if (err)
> +		goto out;
> +
> +	quic_data(&l, tl, strlen(tl));
> +	quic_data(&k, srt.secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> +	srt.type = TLS_CIPHER_AES_GCM_128;
> +	srt.send = 1;
> +	err = quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &k);
> +	if (err)
> +		goto out;
> +	err = quic_crypto_set_secret(crypto, &srt, version, 0);
> +	if (err)
> +		goto out;
> +
> +	quic_data(&l, rl, strlen(rl));
> +	quic_data(&k, srt.secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> +	srt.type = TLS_CIPHER_AES_GCM_128;
> +	srt.send = 0;
> +	err = quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &k);
> +	if (err)
> +		goto out;
> +	err = quic_crypto_set_secret(crypto, &srt, version, 0);
> +out:
> +	memzero_explicit(secret, sizeof(secret));
> +	memzero_explicit(&srt, sizeof(srt));
> +	return err;
> +}

[ ... ]

> +/* Derive and install transmission (TX) packet protection keys for the current key phase.
> + * This involves generating AEAD encryption key, IV, and optionally header protection key.
> + */
> +static int quic_crypto_tx_keys_derive_and_install(struct quic_crypto *crypto)
> +{
> +	struct quic_data srt = {}, k, iv, hp_k = {}, *hp = NULL;
> +	u8 tx_key[QUIC_KEY_LEN], tx_hp_key[QUIC_KEY_LEN] = {};
> +	int err, phase = crypto->key_phase;
> +	u32 keylen, ivlen = QUIC_IV_LEN;
> +
> +	keylen = crypto->cipher->keylen;
> +	quic_data(&srt, crypto->tx_secret, crypto->cipher->secretlen);
> +	quic_data(&k, tx_key, keylen);
> +	quic_data(&iv, crypto->tx_iv[phase], ivlen);
> +	/* Only derive header protection key when not in key update. */
> +	if (!crypto->key_pending)
> +		hp = quic_data(&hp_k, tx_hp_key, keylen);
> +	err = quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv, hp, crypto->version);
> +	if (err)
> +		goto out;
> +	err = crypto_aead_setauthsize(crypto->tx_tfm[phase], QUIC_TAG_LEN);
> +	if (err)
> +		goto out;
> +	err = crypto_aead_setkey(crypto->tx_tfm[phase], tx_key, keylen);
> +	if (err)
> +		goto out;
> +	if (hp) {
> +		err = crypto_skcipher_setkey(crypto->tx_hp_tfm, tx_hp_key, keylen);
> +		if (err)
> +			goto out;
> +	}
> +	pr_debug("%s: k: %16phN, iv: %12phN, hp_k:%16phN\n", __func__, k.data, iv.data, tx_hp_key);
> +out:
> +	memzero_explicit(tx_key, sizeof(tx_key));
> +	memzero_explicit(tx_hp_key, sizeof(tx_hp_key));
> +	return err;
> +}
> +
> +/* Derive and install reception (RX) packet protection keys for the current key phase.
> + * This installs AEAD decryption key, IV, and optionally header protection key.
> + */
> +static int quic_crypto_rx_keys_derive_and_install(struct quic_crypto *crypto)
> +{
> +	struct quic_data srt = {}, k, iv, hp_k = {}, *hp = NULL;
> +	u8 rx_key[QUIC_KEY_LEN], rx_hp_key[QUIC_KEY_LEN] = {};
> +	int err, phase = crypto->key_phase;
> +	u32 keylen, ivlen = QUIC_IV_LEN;
> +
> +	keylen = crypto->cipher->keylen;
> +	quic_data(&srt, crypto->rx_secret, crypto->cipher->secretlen);
> +	quic_data(&k, rx_key, keylen);
> +	quic_data(&iv, crypto->rx_iv[phase], ivlen);
> +	/* Only derive header protection key when not in key update. */
> +	if (!crypto->key_pending)
> +		hp = quic_data(&hp_k, rx_hp_key, keylen);
> +	err = quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv, hp, crypto->version);
> +	if (err)
> +		goto out;
> +	err = crypto_aead_setauthsize(crypto->rx_tfm[phase], QUIC_TAG_LEN);
> +	if (err)
> +		goto out;
> +	err = crypto_aead_setkey(crypto->rx_tfm[phase], rx_key, keylen);
> +	if (err)
> +		goto out;
> +	if (hp) {
> +		err = crypto_skcipher_setkey(crypto->rx_hp_tfm, rx_hp_key, keylen);
> +		if (err)
> +			goto out;
> +	}
> +	pr_debug("%s: k: %16phN, iv: %12phN, hp_k:%16phN\n", __func__, k.data, iv.data, rx_hp_key);
> +out:
> +	memzero_explicit(rx_key, sizeof(rx_key));
> +	memzero_explicit(rx_hp_key, sizeof(rx_hp_key));
> +	return err;
> +}

This isn't a bug, but these two functions are nearly identical.

quic_crypto_tx_keys_derive_and_install() and
quic_crypto_rx_keys_derive_and_install() share the same structure: derive
keys via quic_crypto_keys_derive(), set AEAD auth size and key,
conditionally set the HP skcipher key, pr_debug, and memzero_explicit the
temporaries.

The only differences are the member names accessed (tx_secret vs
rx_secret, tx_iv vs rx_iv, tx_tfm vs rx_tfm, tx_hp_tfm vs rx_hp_tfm).

Could these be unified into a single static helper that takes
direction-specific pointers as parameters?

This would eliminate the duplication and reduce the risk of the two
copies diverging in future maintenance.

[ ... ]