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

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] net/quic/Kconfig selects CRYPTO_HMAC but not a concrete digest
  implementation, while every entry of the new ciphers[] table in…
- [Low] quic_crypto_set_secret() consumes the uAPI struct
  quic_crypto_secret but never reads `level` or `reserved`, and honours…

--

quic: add crypto key derivation and installation

This adds net/quic/crypto.c with the QUIC key schedule: HKDF-Extract and
HKDF-Expand-Label helpers, Initial secret derivation, traffic secret
installation, key update, and stateless reset token derivation.  The new
struct quic_crypto is embedded per encryption level in struct quic_sock and
freed from the new quic_sock_destruct().

> diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> new file mode 100644
> index 0000000000000..910557b680524
> --- /dev/null
> +++ b/net/quic/crypto.c
> @@ -0,0 +1,567 @@

[ ... ]

> +static const struct quic_cipher
> +ciphers[QUIC_CIPHER_MAX + 1 - QUIC_CIPHER_MIN] = {
> +	CIPHER_DESC(TLS_CIPHER_AES_GCM_128,
> +		    "gcm(aes)", "ecb(aes)", "hmac(sha256)"),
> +	CIPHER_DESC(TLS_CIPHER_AES_GCM_256,
> +		    "gcm(aes)", "ecb(aes)", "hmac(sha384)"),
> +	CIPHER_DESC(TLS_CIPHER_AES_CCM_128,
> +		    "ccm(aes)", "ecb(aes)", "hmac(sha256)"),
> +	CIPHER_DESC(TLS_CIPHER_CHACHA20_POLY1305,
> +		    "rfc7539(chacha20,poly1305)", "chacha20", "hmac(sha256)"),
> +};
> +
> +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;

[Severity: Medium]

Every entry of ciphers[] requests either hmac(sha256) or hmac(sha384), but
net/quic/Kconfig does not select a digest implementation:

	select CRYPTO
	select CRYPTO_HMAC
	select CRYPTO_AES
	select CRYPTO_GCM
	select CRYPTO_CCM
	select CRYPTO_CHACHA20POLY1305
	select NET_UDP_TUNNEL

CRYPTO_HMAC is only the template (it selects CRYPTO_HASH and
CRYPTO_MANAGER), while CRYPTO_SHA256 and CRYPTO_SHA512 are independent
tristates without default y.

With IP_QUIC=y/m and CRYPTO_SHA256=n, does crypto_alloc_shash(cipher->shash,
0, 0) in quic_crypto_set_cipher() fail with -ENOENT, so that
quic_crypto_set_secret() and quic_crypto_initial_keys_install() can never
install keys and no connection can be established?

Should IP_QUIC also select CRYPTO_SHA256 and CRYPTO_SHA512 (the latter for
the hmac(sha384) entry)?

[ ... ]

> +int quic_crypto_set_secret(struct quic_crypto *crypto,
> +			   struct quic_crypto_secret *srt, u32 version)
> +{
> +	const struct quic_cipher *cipher;
> +	u8 phase = crypto->key_phase;
> +	int err;
> +
> +	/* If no cipher has been initialized yet, set it up. */
> +	if (!crypto->cipher) {
> +		err = quic_crypto_set_cipher(crypto, srt->type);
> +		if (err)
> +			return err;
> +	}
> +	cipher = crypto->cipher;

[Severity: Low]

This isn't a bug in the current series, since quic_do_setsockopt() still
returns -EOPNOTSUPP and the only in-tree producer,
quic_crypto_initial_keys_install(), sets srt.type = TLS_CIPHER_AES_GCM_128 on
both calls.

Still, srt->type is only consulted while crypto->cipher is NULL, and
srt->level and srt->reserved are never read at all.  Once the setsockopt path
is wired up, would a second call carrying a different TLS_CIPHER_* value be
silently served with the already installed cipher, copying cipher->secretlen
bytes out of the caller's secret buffer instead of returning -EINVAL?

Would it make sense to reject srt->type disagreeing with crypto->cipher_type
here?

[ ... ]

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