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

[Paolo Abeni <pabeni@redhat.com>]

From: AI Reviewer <ai@example.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.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/
---
quic: add crypto packet encryption and decryption

This patch adds AEAD-based packet encryption/decryption helpers, QUIC
header protection, and Retry/address-validation token generation and
verification primitives.

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> index c68a0b570c72..77e633096c2a 100644
> --- a/net/quic/crypto.c
> +++ b/net/quic/crypto.c

[ ... ]

> +/* Retry Packet Integrity. */
> +int quic_crypto_get_retry_tag(struct quic_crypto *crypto, struct sk_buff *skb,
> +			      struct quic_conn_id *odcid, u32 version, u8 *tag)
> +{
> +	/* Reuse RX AEAD (phase 1) in Initial crypto. */
> +	struct crypto_aead *tfm = crypto->rx_tfm[1];
> +	u8 *pseudo_retry, *p, *iv, *key;
> +	DECLARE_CRYPTO_WAIT(wait);
> +	struct aead_request *req;
> +	struct scatterlist *sg;
> +	u32 plen;
> +	int err;
> +
> +	if (skb->len < QUIC_TAG_LEN)
> +		return -EINVAL;

[Low]
The guard rejects skb->len < QUIC_TAG_LEN, but a later comment in the
same function says:

> +	/* The caller must ensure skb->len > QUIC_TAG_LEN. */
> +	plen = 1 + odcid->len + skb->len - QUIC_TAG_LEN;

Should the check be <= QUIC_TAG_LEN, or should the comment say >= to
match the strict-less-than test?

In the boundary case where skb->len == QUIC_TAG_LEN, plen becomes
1 + odcid->len, and quic_put_data(p, skb->data, skb->len - QUIC_TAG_LEN)
copies zero bytes, so the integrity tag would be computed over a
pseudo-packet containing only the ODCID length and ODCID with no actual
Retry header bytes.  Is that intended?

[ ... ]

> +/* Derives a key and IV using HKDF, configures the AEAD transform and performs
> + * AEAD encryption/decryption for the provided token.
> + */
> +static int quic_crypto_token_protect(struct quic_crypto *crypto, u8 *token,
> +				     u32 len, u32 adlen, bool enc)
> +{
> +	u8 key[TLS_CIPHER_AES_GCM_128_KEY_SIZE], iv[QUIC_IV_LEN], *tiv;
> +	/* Reuse TX AEAD (phase 1) in Initial crypto. */
> +	struct crypto_aead *tfm = crypto->tx_tfm[1];
> +	u32 extra = enc ? QUIC_TAG_LEN : 0;
> +	struct quic_data srt = {}, k, i;
> +	DECLARE_CRYPTO_WAIT(wait);
> +	struct aead_request *req;
> +	struct scatterlist *sg;
> +	void *ctx = NULL;
> +	int err;
> +
> +	quic_data(&srt, quic_random_data, QUIC_RANDOM_DATA_LEN);
> +	quic_data(&k, key, TLS_CIPHER_AES_GCM_128_KEY_SIZE);
> +	quic_data(&i, iv, QUIC_IV_LEN);
> +	err = quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &i, NULL,
> +				      QUIC_VERSION_V1);

[Critical]
Can this lead to AES-GCM nonce reuse across all generated tokens?

quic_random_data is a module-global buffer initialized once in
quic_crypto_init() via get_random_bytes() and never refreshed:

    static u8 quic_random_data[QUIC_RANDOM_DATA_LEN] __read_mostly;

quic_crypto_keys_derive() runs HKDF-Expand-Label over that constant
secret with constant labels "quic key" and "quic iv", so both the
derived key and the derived IV are deterministic and identical on every
invocation of quic_crypto_token_protect().

> +	if (err)
> +		goto out;
> +	err = crypto_aead_setauthsize(tfm, QUIC_TAG_LEN);
> +	if (err)
> +		goto out;
> +	err = crypto_aead_setkey(tfm, key, TLS_CIPHER_AES_GCM_128_KEY_SIZE);
> +	if (err)
> +		goto out;
> +	ctx = quic_crypto_aead_mem_alloc(tfm, 0, &tiv, &req, &sg, 1);
> +	if (!ctx) {
> +		err = -ENOMEM;
> +		goto out;
> +	}
> +	memcpy(tiv, iv, QUIC_IV_LEN);
> +
> +	sg_init_one(sg, token, len);
> +	aead_request_set_tfm(req, tfm);
> +	aead_request_set_ad(req, adlen);
> +	aead_request_set_crypt(req, sg, sg, len - adlen - extra, tiv);

Every call ends up using the same (K, N) pair under AES-GCM, with no
per-token random prefix and no packet-number XOR like
quic_crypto_payload_protect() does via cb->number.

NIST SP 800-38D forbids reuse of (K, N) under GCM; from two valid
ciphertexts an attacker can recover the GHASH authentication subkey H
and forge tokens for arbitrary client addresses, which would defeat
the address validation in quic_crypto_verify_token() and the
anti-amplification protections in RFC 9000 section 8.

Could a unique per-token nonce be derived (for example from a random
prefix stored in the token itself), or a nonce-misuse-resistant AEAD
be used here instead?

> +	aead_request_set_callback(req, CRYPTO_TFM_REQ_MAY_BACKLOG,
> +				  crypto_req_done, &wait);
> +	err = enc ? crypto_aead_encrypt(req) : crypto_aead_decrypt(req);
> +	if (err == -EINPROGRESS || err == -EBUSY)
> +		err = crypto_wait_req(err, &wait);
> +
> +out:
> +	memzero_explicit(key, sizeof(key));
> +	memzero_explicit(iv, sizeof(iv));
> +	kfree_sensitive(ctx);
> +	return err;
> +}

[ ... ]
-- 
This is an AI-generated review.