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

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.

[ ... ]

[ ... ]

[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?

[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?

[ ... ]

[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.

[ ... ]

[ ... ]

[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?

[ ... ]

[ ... ]

[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