Re: [net-next,v10,12/15] quic: add crypto packet encryption and decryption

[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 packet encryption and decryption

This patch adds core support for packet-level encryption and decryption
using AEAD, including both payload protection and QUIC header protection.
It introduces helpers to encrypt and decrypt packets for secure QUIC
packet exchange and address validation.

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> index a9de418e604e..a84fd8d7bf93 100644
> --- a/net/quic/crypto.c
> +++ b/net/quic/crypto.c

[ ... ]

> @@ -231,6 +562,137 @@ static struct quic_cipher ciphers[QUIC_CIPHER_MAX + 1 - QUIC_CIPHER_MIN] = {

[ ... ]

> +/* Encrypts a QUIC packet before transmission.  This function performs AEAD encryption of
> + * the packet payload and applies header protection. It handles key phase tracking and key
> + * update timing.
> + *
> + * Return: 0 on success, or a negative error code.
> + */
> +int quic_crypto_encrypt(struct quic_crypto *crypto, struct sk_buff *skb)
> +{
> +	u8 *iv, cha, ccm, phase = crypto->key_phase;
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	int err;
> +
> +	cb->key_phase = phase;
> +	iv = crypto->tx_iv[phase];
> +	/* Packet payload is already encrypted (e.g., resumed from async), proceed to header
> +	 * protection only.
> +	 */
> +	if (cb->resume)
> +		goto out;
> +
> +	/* If a key update is pending and this is the first packet using the new key, save the
> +	 * current time. Later used to clear old keys after some time has passed (see
> +	 * quic_crypto_decrypt()).
> +	 */
> +	if (crypto->key_pending && !crypto->key_update_send_time)
> +		crypto->key_update_send_time = quic_ktime_get_us();
> +
> +	ccm = quic_crypto_is_cipher_ccm(crypto);
> +	err = quic_crypto_payload_encrypt(crypto->tx_tfm[phase], skb, iv, ccm);
> +	if (err)
> +		return err;
> +out:
> +	cha = quic_crypto_is_cipher_chacha(crypto);
> +	return quic_crypto_header_encrypt(crypto->tx_hp_tfm, skb, cha);
> +}

This function calls quic_crypto_payload_encrypt(). Looking at that
function:

> +/* AEAD Usage. */
> +static int quic_crypto_payload_encrypt(struct crypto_aead *tfm, struct sk_buff *skb,
> +				       u8 *tx_iv, bool ccm)
> +{
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	u8 *iv, i, nonce[QUIC_IV_LEN];
> +	struct aead_request *req;
> +	struct sk_buff *trailer;
> +	struct scatterlist *sg;
> +	struct quichdr *hdr;
> +	u32 nsg, hlen, len;
> +	void *ctx;
> +	__be64 n;
> +	int err;
> +
> +	len = skb->len;
> +	err = skb_cow_data(skb, QUIC_TAG_LEN, &trailer);
> +	if (err < 0)
> +		return err;
> +	nsg = (u32)err;
> +	pskb_put(skb, trailer, QUIC_TAG_LEN);
> +	hdr = quic_hdr(skb);
> +	hdr->key = cb->key_phase;
> +
> +	ctx = quic_crypto_aead_mem_alloc(tfm, 0, &iv, &req, &sg, nsg);
> +	if (!ctx)
> +		return -ENOMEM;

If the kzalloc(GFP_ATOMIC) inside quic_crypto_aead_mem_alloc() fails,
this returns -ENOMEM without rolling back the SKB modifications. The
SKB is now permanently enlarged by QUIC_TAG_LEN (16 bytes) with a
modified hdr->key.

Compare with quic_crypto_payload_decrypt():

> +static int quic_crypto_payload_decrypt(struct crypto_aead *tfm, struct sk_buff *skb,
> +				       u8 *rx_iv, bool ccm)
> +{
> +	struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> +	u8 *iv, i, nonce[QUIC_IV_LEN];
> +	struct aead_request *req;
> +	int nsg, hlen, len, err;
> +	struct scatterlist *sg;
> +	void *ctx;
> +	__be64 n;
> +
> +	len = cb->length + cb->number_offset;
> +	hlen = cb->number_offset + cb->number_len;
> +	if (len - hlen < QUIC_TAG_LEN)
> +		return -EINVAL;
> +	nsg = 1; /* skb is already linearized in quic_packet_rcv(). */
> +	ctx = quic_crypto_aead_mem_alloc(tfm, 0, &iv, &req, &sg, nsg);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	sg_init_table(sg, nsg);
> +	err = skb_to_sgvec(skb, sg, 0, len);

In the decrypt path, quic_crypto_aead_mem_alloc() is called before any
SKB modifications. Should quic_crypto_payload_encrypt() do the same to
avoid leaving the SKB in a modified state if allocation fails?

[ ... ]

> +	/* Similar logic to quic_crypto_payload_encrypt(). */
> +	memcpy(nonce, rx_iv, QUIC_IV_LEN);
> +	n = cpu_to_be64(cb->number);
> +	for (i = 0; i < sizeof(n); i++)
> +		nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((u8 *)&n)[i];

This isn't a bug, but there's duplicated logic between
quic_crypto_payload_encrypt() and quic_crypto_payload_decrypt(). The
AEAD nonce construction sequence (memcpy nonce from IV, XOR with be64
packet number, set CCM byte) appears in both functions.

In quic_crypto_payload_encrypt():

> +	hlen = cb->number_offset + cb->number_len;
> +	memcpy(nonce, tx_iv, QUIC_IV_LEN);
> +	n = cpu_to_be64(cb->number);
> +	for (i = 0; i < sizeof(n); i++)
> +		nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((u8 *)&n)[i];
> +
> +	/* For CCM based ciphers, first byte of IV is a constant. */
> +	iv[0] = TLS_AES_CCM_IV_B0_BYTE;
> +	memcpy(&iv[ccm], nonce, QUIC_IV_LEN);

And in quic_crypto_payload_decrypt():

> +	/* Similar logic to quic_crypto_payload_encrypt(). */
> +	memcpy(nonce, rx_iv, QUIC_IV_LEN);
> +	n = cpu_to_be64(cb->number);
> +	for (i = 0; i < sizeof(n); i++)
> +		nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((u8 *)&n)[i];
> +
> +	iv[0] = TLS_AES_CCM_IV_B0_BYTE;
> +	memcpy(&iv[ccm], nonce, QUIC_IV_LEN);

The AEAD request teardown pattern (EINPROGRESS check, kfree_sensitive,
memzero_explicit) is also duplicated. Would a shared static helper for
nonce construction reduce the risk of the two copies diverging during
maintenance?

[ ... ]