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

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

[ ... ]

+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