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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The AEAD integrity limit that this patch enforces does not
  count failures across the whole connection, as RFC 9001 §6.6 requires.
- [Medium] quic_crypto_generate_token() writes the raw 64-bit
  quic_ktime_get_us() timestamp (ktime_get(), i.e. CLOCK_MONOTONIC in µs…
- [Low] The kernel-doc of quic_crypto_verify_token() says "Return: 0 if
  the token is valid, -EINVAL if invalid, or another negative error…
- [Low] quic_crypto_decrypt() counts every non-EINPROGRESS failure of
  quic_crypto_payload_protect() toward the AEAD integrity limit, not…
- [Low] async_pending[] is used as a quiescence check before re-keying a
  tfm, but both sides use unordered atomics.

--

quic: add crypto packet encryption and decryption

This patch adds AEAD payload protection and QUIC header protection for
TX and RX via quic_crypto_encrypt() and quic_crypto_decrypt(). It also
adds helpers to compute the Retry integrity tag, and to generate and
verify Retry/address validation tokens.

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> index 4bb6acea5112a..fa326cc9266a8 100644
> --- a/net/quic/crypto.c
> +++ b/net/quic/crypto.c
> @@ -270,6 +271,484 @@ ciphers[QUIC_CIPHER_MAX + 1 - QUIC_CIPHER_MIN] = {

[ ... ]

> +int quic_crypto_decrypt(struct quic_crypto *crypto, struct sk_buff *skb,
> +			gfp_t gfp)
> +{

[ ... ]

> +	phase = cb->key_phase;
> +	if (phase != crypto->key_phase && !crypto->key_pending) {
> +		if (!crypto->send_ready) /* Not ready for key update. */
> +			return -EINVAL;
> +		if (!cb->backlog) /* Key update requires process context. */
> +			return -EKEYREVOKED;
> +		/* Cannot do key update while async crypto is in progress. */
> +		if (unlikely(atomic_read(&crypto->async_pending[phase])))
> +			return -EBUSY;
> +		err = quic_crypto_key_update(crypto); /* Perform key update. */

[Severity: Low]
Is the ordering here strong enough for async_pending[] to work as a
quiescence check before re-keying?

quic_crypto_done() signals completion with a plain atomic_dec(). Per
Documentation/atomic_t.txt, that operation is unordered:

    atomic_dec(&crypto->async_pending[cb->key_phase]);

This side reads the counter with a relaxed atomic_read(). When it sees 0,
it calls quic_crypto_key_update(), which rewrites the key schedule through
crypto_aead_setkey().

Nothing orders the completing CPU's loads of the key schedule during the
AEAD operation before its decrement. The socket lock does not cover this,
because the decrement happens before cb->crypto_done() takes the lock
again.

On weakly ordered architectures, can the re-keying CPU see 0 and overwrite
the key while the completing request's reads are still unordered? Would
atomic_dec_return_release(), or smp_mb__before_atomic() plus atomic_dec(),
in quic_crypto_done() paired with atomic_read_acquire() here be needed?

> +		if (err) {
> +			cb->errcode = QUIC_TRANSPORT_ERROR_KEY_UPDATE;
> +			return err;
> +		}
> +		cb->sync = 1;
> +		cb->key_update = 1; /* Mark packet as triggering key update. */
> +	}
> +
> +	err = quic_crypto_payload_protect(crypto, skb, false, gfp);
> +	if (err) {
> +		if (err == -EINPROGRESS)
> +			return err;
> +		/* When using the old keys can not decrypt the packets, the
> +		 * peer might start another key_update. Thus, clear the last
> +		 * key_pending so that next packets will trigger the new
> +		 * key-update.
> +		 */
> +		if (crypto->key_pending && phase != crypto->key_phase) {
> +			crypto->key_pending = 0;
> +			crypto->key_update_time = 0;
> +			crypto->key_update_send_time = 0;
> +		}
> +		goto err;
> +	}

[Severity: Low]
Should every quic_crypto_payload_protect() failure count toward the AEAD
integrity limit?

All of these reach the err label and increment rx_fails[cb->key_phase]:

  - -ENOMEM from quic_crypto_aead_mem_alloc()
  - a negative skb_to_sgvec() result
  - on the resume path, any async engine error delivered in
    cb->crypto_err

RFC 9001 section 6.6 counts only received packets that fail
authentication.

Under sustained allocation failure, could a legitimate connection end up
with -EKEYEXPIRED (AEAD_LIMIT_REACHED) instead of just dropping packets?

[ ... ]

> +	return 0;
> +err:
> +	if (++crypto->rx_fails[cb->key_phase] >= crypto->cipher->rxlimit)
> +		err = -EKEYEXPIRED;
> +	return err;
> +}

[Severity: Medium]
Does this enforce the integrity limit across the whole connection, as
RFC 9001 section 6.6 requires?

The comment on the QUIC_AEAD_RX_* constants cites that section. It says
endpoints MUST count packets that fail authentication during the lifetime
of a connection, across all keys.

Two things seem to weaken the limit here.

First, cb->key_phase comes from the key bit after header protection is
removed, and the packet has not been authenticated yet. Forged
short-header packets therefore spread across both rx_fails[] slots, which
allows about 2 * rxlimit forgeries.

Second, quic_crypto_keys_derive_and_install() resets the slot on every RX
key install. That includes each key update reached through
quic_crypto_decrypt()->quic_crypto_key_update():

net/quic/crypto.c:quic_crypto_keys_derive_and_install() {
    ...
	if (rx) {
		crypto->rx_fails[phase] = 0;
    ...
}

For AES-CCM (rxlimit 2965820, about 2^21.5), an off-path sender would get
roughly (2 + number of key updates) * rxlimit forgery attempts.

Would a single connection-wide failure counter, not indexed by key phase
and not reset on key install, be more appropriate? tx_count could stay
per key.

[ ... ]

> @@ -560,6 +1039,234 @@ int quic_crypto_initial_keys_install(struct quic_crypto *crypto,

[ ... ]

> +int quic_crypto_generate_token(struct quic_crypto *crypto, void *addr,
> +			       u32 addrlen, struct quic_conn_id *conn_id,
> +			       u8 *token, u32 *tlen)
> +{
> +	u8 *token_buf, *p, flag = *token;
> +	u64 ts = quic_ktime_get_us();
> +	u32 len, tslen = sizeof(ts);
> +	int err;
> +
> +	len = sizeof(flag) + addrlen + tslen + conn_id->len + QUIC_TAG_LEN;
> +	token_buf = kmalloc(len, GFP_KERNEL);
> +	if (!token_buf)
> +		return -ENOMEM;
> +
> +	p = token_buf;
> +	p = quic_put_int(p, flag, sizeof(flag));
> +	p = quic_put_data(p, addr, addrlen);
> +	p = quic_put_int(p, ts, tslen);
> +	quic_put_data(p, conn_id->data, conn_id->len);

[Severity: Medium]
Does this expose the server's uptime to any client that receives a token?

ts is the raw ktime_get() value, in microseconds since boot.
quic_crypto_token_protect() only authenticates the flag, address and
timestamp as associated data. It does not encrypt them:

	aead_request_set_ad(req, adlen);
	aead_request_set_crypt(req, sg, sg, len - adlen - extra, nonce);

Tokens are sent to unauthenticated clients in Retry packets and NEW_TOKEN
frames, so the monotonic clock can be read from them in plaintext. TCP
avoids a similar leak with per-connection timestamp offsets
(secure_tcp_ts_off).

The v14 change moved the timestamp from the encrypted part into the AAD
so it could be used for the nonce. That change appears to have introduced
this exposure.

The serialized address is also sent in plaintext. For IPv6,
quic_v6_get_msg_addrs() in net/quic/family.c sets:

	sa->v6.sin6_scope_id = skb->skb_iif;

Would the server's ingress ifindex end up in the token as well?

[ ... ]

> +/* Validate a Retry or address validation token.
> + *
> + * Decrypts and authenticates the token, checks the client address and validates
> + * the timestamp against the timeout selected by the token flag. For a Retry
> + * token, extracts the original destination connection ID.
> + *
> + * Return: 0 if the token is valid, -EINVAL if invalid, or another negative
> + * error code.
> + */
> +int quic_crypto_verify_token(struct quic_crypto *crypto, void *addr,
> +			     u32 addrlen, struct quic_conn_id *conn_id,
> +			     u8 *token, u32 len)
> +{

[ ... ]

> +	err = quic_crypto_token_protect(crypto, token_buf, len,
> +					sizeof(flag) + addrlen + tslen, false);
> +	if (err)
> +		goto out;
> +
> +	err = -EINVAL;

[Severity: Low]
The kernel-doc says -EINVAL is returned for an invalid token. Does a
forged or tampered token actually return -EINVAL?

A tag mismatch makes crypto_aead_decrypt() return -EBADMSG. That value is
returned unchanged through the goto out above, before err is set to
-EINVAL. A token key that was never set would give -ENOKEY in the same
way.

Only the length precheck and the address, timestamp and ODCID length
checks return -EINVAL.

Should the comment be updated, or should authentication failures be
mapped to -EINVAL?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791227050.git.lucien.xin%40gmail.com