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

[netdev-bot+sashiko@kernel.org]

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

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

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] quic_crypto_set_cipher() (net/quic/crypto.c:272) stores seven
  freshly allocated transforms (secret_tfm, rx_tfm[0..1], rx_hp_tfm,…
- [Low] The commit message that stays in git history leaves out the
  socket lifecycle changes in net/quic/socket.c.
- [Low] quic_crypto_set_cipher() allocates the header-protection
  transforms with crypto_alloc_sync_skcipher(), which returns struct…

--

quic: add crypto key derivation and installation

This patch adds net/quic/crypto.c. It provides HKDF helpers, cipher
transform allocation, traffic secret installation, key update and
stateless reset token derivation for QUIC. It also allocates one
struct quic_crypto per encryption level in quic_init_sock(), and a new
sk_destruct callback defers freeing those contexts to quic_wq.

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

[ ... ]

> - quic_crypto_derive_secret(): Generate token for stateless reset or
>   master key for session resumption.
>
> These mechanisms are essential for establishing and maintaining secure
> communication throughout the QUIC connection lifecycle.

[Severity: Low]
The commit message describes this patch only as adding crypto helpers.
Could it also mention the socket lifecycle changes in net/quic/socket.c?

The patch also does the following:

  - installs a new quic_sock_destruct() as sk->sk_destruct in
    quic_init_sock()
  - allocates QUIC_CRYPTO_MAX struct quic_crypto contexts per socket
    with kzalloc_obj()
  - defers transform teardown to quic_crypto_free_work() on quic_wq,
    which runs after the socket has been destroyed

The function list also leaves out the new exported helper
quic_crypto_set_token_secret().

These details appear only in the v18 notes below the --- line, and
git am drops that text.

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> new file mode 100644
> index 0000000000000..c4bf99b3e8239
> --- /dev/null
> +++ b/net/quic/crypto.c

[ ... ]

> +int quic_crypto_set_cipher(struct quic_crypto *crypto, u32 type)
> +{
> +	const struct quic_cipher *cipher;
> +	void *tfm;
> +	int err;
> +
> +	if (type < QUIC_CIPHER_MIN || type > QUIC_CIPHER_MAX)
> +		return -EINVAL;
> +
> +	cipher = &ciphers[type - QUIC_CIPHER_MIN];
> +	tfm = crypto_alloc_shash(cipher->shash, 0, 0);
> +	if (IS_ERR(tfm))
> +		return PTR_ERR(tfm);
> +	crypto->secret_tfm = tfm;

[ ... ]

> +	tfm = crypto_alloc_sync_skcipher(cipher->skc, 0, 0);
> +	if (IS_ERR(tfm)) {
> +		err = PTR_ERR(tfm);
> +		goto err;
> +	}
> +	crypto->rx_hp_tfm = tfm;

[Severity: Low]
Are the types for the header protection transforms consistent here?

crypto_alloc_sync_skcipher() returns a struct crypto_sync_skcipher *.
The result passes through the void *tfm local into rx_hp_tfm, and
tx_hp_tfm is handled the same way. Both fields are declared as struct
crypto_skcipher * in net/quic/crypto.h.

quic_crypto_free() then releases them with crypto_free_skcipher()
instead of crypto_free_sync_skcipher():

	if (crypto->rx_hp_tfm)
		crypto_free_skcipher(crypto->rx_hp_tfm);
	if (crypto->tx_hp_tfm)
		crypto_free_skcipher(crypto->tx_hp_tfm);

This works today only because struct crypto_sync_skcipher wraps a
struct crypto_skcipher base at offset 0. Because tfm is a void *, the
compiler cannot catch the mismatch. The mismatch is still present at
the end of the series, where quic_crypto_header_protect() uses these
fields as struct crypto_skcipher *.

Should the fields be declared as struct crypto_sync_skcipher * and
freed with crypto_free_sync_skcipher()?

[ ... ]

> +	crypto->tx_hp_tfm = tfm;
> +
> +	crypto->cipher = cipher;
> +	crypto->cipher_type = type;
> +	return 0;
> +err:
> +	quic_crypto_free(crypto);
> +	return err;
> +}

[Severity: Medium]
Can this error path destroy state that this call did not create?

quic_crypto_set_cipher() stores seven newly allocated transforms
without checking whether the context already holds any. On failure it
calls quic_crypto_free() on the whole context. That frees every
transform and then zeroes the structure up to send_offset:

	memzero_explicit(crypto, offsetof(struct quic_crypto, send_offset));

If this ran on a context that was already set up, the old transforms
would leak. If that second call then failed, it would also wipe the
live keys, the token secret in tx_secret[1], and the cipher,
send_ready and recv_ready state.

In this series the only caller is quic_crypto_set_secret(). It calls
quic_crypto_set_cipher() only when crypto->cipher is NULL, so this
path does not appear to be reachable yet.

The notes say later patches call quic_crypto_set_cipher() directly from
quic_inet_listen(), quic_connect() and quic_accept(). Those patches are
not part of this series.

Would it be safer to reject a context that already has transforms, and
to unwind only the allocations made by this call?

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