From: Xin Long <lucien.xin@gmail.com>
To: Paolo Abeni <pabeni@redhat.com>
Cc: hepengtao@xiaomi.com, kuba@kernel.org, jlayton@kernel.org,
metze@samba.org, davem@davemloft.net,
kernel-tls-handshake@lists.linux.dev, horms@kernel.org,
hare@suse.de, aahringo@redhat.com, tfanelli@redhat.com,
mail@johnericson.me, andrew.gospodarek@broadcom.com,
linkinjeon@kernel.org, dhowells@redhat.com, matttbe@kernel.org,
xiyou.wangcong@gmail.com, dreibh@simula.no, jbaron@akamai.com,
quic@lists.linux.dev, pc@manguebit.com, marc@fiuczynski.com,
edumazet@google.com, alibuda@linux.alibaba.com,
steved@redhat.com, illiliti@protonmail.com, tom@talpey.com,
marcelo.leitner@gmail.com, sd@queasysnail.net,
linux-cifs@vger.kernel.org, netdev@vger.kernel.org,
mbuhl@openbsd.org, smfrench@gmail.com, chuck.lever@oracle.com,
daniel@haxx.se
Subject: Re: [net-next,v10,11/15] quic: add crypto key derivation and installation
Date: Wed, 4 Mar 2026 16:58:01 -0500 [thread overview]
Message-ID: <CADvbK_cWP-S1VdsfZJCc_iTxaeVnGTbrXrNsVdWTgYjdqLjwaA@mail.gmail.com> (raw)
In-Reply-To: <20260303083233.34650-1-pabeni@redhat.com>
On Tue, Mar 3, 2026 at 3:33 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
>
> For local reproduction steps see https://netdev-ai.bots.linux.dev/ai-local.html
> ---
> 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).
> It provides helpers to derive and install initial secrets, set traffic
> secrets and install the corresponding keys, and perform key updates to
> enable forward secrecy.
>
> > diff --git a/net/quic/Makefile b/net/quic/Makefile
> > index 9d8e18297911..58bb18f7926d 100644
> > --- a/net/quic/Makefile
> > +++ b/net/quic/Makefile
> > @@ -6,4 +6,4 @@
> > obj-$(CONFIG_IP_QUIC) += quic.o
> >
> > quic-y := common.o family.o protocol.o socket.o stream.o connid.o path.o \
> > - cong.o pnspace.o
> > + cong.o pnspace.o crypto.o
> > diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> > new file mode 100644
> > index 000000000000..a9de418e604e
> > --- /dev/null
> > +++ b/net/quic/crypto.c
>
> [ ... ]
>
> > +/* Initial Secrets. */
> > +int quic_crypto_initial_keys_install(struct quic_crypto *crypto, struct quic_conn_id *conn_id,
> > + u32 version, bool is_serv)
> > +{
> > + u8 secret[TLS_CIPHER_AES_GCM_128_SECRET_SIZE];
> > + struct quic_data salt, s, k, l, dcid, z = {};
> > + struct quic_crypto_secret srt = {};
> > + char *tl, *rl, *sal;
> > + int err;
> > +
> > + if (is_serv) {
> > + rl = "client in";
> > + tl = "server in";
> > + } else {
> > + tl = "client in";
> > + rl = "server in";
> > + }
> > + sal = QUIC_INITIAL_SALT_V1;
> > + if (version == QUIC_VERSION_V2)
> > + sal = QUIC_INITIAL_SALT_V2;
> > + quic_data(&salt, sal, QUIC_INITIAL_SALT_LEN);
> > + quic_data(&dcid, conn_id->data, conn_id->len);
> > + quic_data(&s, secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> > + err = quic_crypto_hkdf_extract(crypto->secret_tfm, &salt, &dcid, &s);
>
> Is crypto->secret_tfm guaranteed to be initialized at this point?
>
> Looking at the function flow, this line uses crypto->secret_tfm before
> quic_crypto_set_secret() below gets a chance to initialize the cipher:
>
> quic_crypto_initial_keys_install() {
> err = quic_crypto_hkdf_extract(crypto->secret_tfm, ...); /* line 490 */
> ...
> err = quic_crypto_set_secret(crypto, &srt, version, 0); /* line 497 */
> }
>
> If quic_crypto_set_cipher() hasn't been called on this struct quic_crypto
> yet, crypto->secret_tfm is NULL.
>
> This contrasts with quic_crypto_set_secret(), which has lazy initialization:
>
> quic_crypto_set_secret() {
> if (!crypto->cipher) {
> err = quic_crypto_set_cipher(crypto, srt->type, flag);
> if (err)
> return err;
> }
> ...
> }
>
> Without this initialization, quic_crypto_hkdf_extract() flows through
> hkdf_extract() -> crypto_shash_setkey(NULL, ...), causing a NULL pointer
> dereference.
>
> Would it make sense to call quic_crypto_set_cipher() before using
> secret_tfm, or to reorder the function so quic_crypto_set_secret() is
> called first?
>
secret_tfm will always set in quic_connect(), quic_accept() and
quic_inet_listen(),
quic_crypto_initial_keys_install() will be only called after one of
these 3 functions
is called.
The patchset-2 (the following series) will give you this completeness.
> > + if (err)
> > + goto out;
> > +
> > + quic_data(&l, tl, strlen(tl));
> > + quic_data(&k, srt.secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> > + srt.type = TLS_CIPHER_AES_GCM_128;
> > + srt.send = 1;
> > + err = quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &k);
> > + if (err)
> > + goto out;
> > + err = quic_crypto_set_secret(crypto, &srt, version, 0);
> > + if (err)
> > + goto out;
> > +
> > + quic_data(&l, rl, strlen(rl));
> > + quic_data(&k, srt.secret, TLS_CIPHER_AES_GCM_128_SECRET_SIZE);
> > + srt.type = TLS_CIPHER_AES_GCM_128;
> > + srt.send = 0;
> > + err = quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &k);
> > + if (err)
> > + goto out;
> > + err = quic_crypto_set_secret(crypto, &srt, version, 0);
> > +out:
> > + memzero_explicit(secret, sizeof(secret));
> > + memzero_explicit(&srt, sizeof(srt));
> > + return err;
> > +}
>
> [ ... ]
>
> > +/* Derive and install transmission (TX) packet protection keys for the current key phase.
> > + * This involves generating AEAD encryption key, IV, and optionally header protection key.
> > + */
> > +static int quic_crypto_tx_keys_derive_and_install(struct quic_crypto *crypto)
> > +{
> > + struct quic_data srt = {}, k, iv, hp_k = {}, *hp = NULL;
> > + u8 tx_key[QUIC_KEY_LEN], tx_hp_key[QUIC_KEY_LEN] = {};
> > + int err, phase = crypto->key_phase;
> > + u32 keylen, ivlen = QUIC_IV_LEN;
> > +
> > + keylen = crypto->cipher->keylen;
> > + quic_data(&srt, crypto->tx_secret, crypto->cipher->secretlen);
> > + quic_data(&k, tx_key, keylen);
> > + quic_data(&iv, crypto->tx_iv[phase], ivlen);
> > + /* Only derive header protection key when not in key update. */
> > + if (!crypto->key_pending)
> > + hp = quic_data(&hp_k, tx_hp_key, keylen);
> > + err = quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv, hp, crypto->version);
> > + if (err)
> > + goto out;
> > + err = crypto_aead_setauthsize(crypto->tx_tfm[phase], QUIC_TAG_LEN);
> > + if (err)
> > + goto out;
> > + err = crypto_aead_setkey(crypto->tx_tfm[phase], tx_key, keylen);
> > + if (err)
> > + goto out;
> > + if (hp) {
> > + err = crypto_skcipher_setkey(crypto->tx_hp_tfm, tx_hp_key, keylen);
> > + if (err)
> > + goto out;
> > + }
> > + pr_debug("%s: k: %16phN, iv: %12phN, hp_k:%16phN\n", __func__, k.data, iv.data, tx_hp_key);
> > +out:
> > + memzero_explicit(tx_key, sizeof(tx_key));
> > + memzero_explicit(tx_hp_key, sizeof(tx_hp_key));
> > + return err;
> > +}
> > +
> > +/* Derive and install reception (RX) packet protection keys for the current key phase.
> > + * This installs AEAD decryption key, IV, and optionally header protection key.
> > + */
> > +static int quic_crypto_rx_keys_derive_and_install(struct quic_crypto *crypto)
> > +{
> > + struct quic_data srt = {}, k, iv, hp_k = {}, *hp = NULL;
> > + u8 rx_key[QUIC_KEY_LEN], rx_hp_key[QUIC_KEY_LEN] = {};
> > + int err, phase = crypto->key_phase;
> > + u32 keylen, ivlen = QUIC_IV_LEN;
> > +
> > + keylen = crypto->cipher->keylen;
> > + quic_data(&srt, crypto->rx_secret, crypto->cipher->secretlen);
> > + quic_data(&k, rx_key, keylen);
> > + quic_data(&iv, crypto->rx_iv[phase], ivlen);
> > + /* Only derive header protection key when not in key update. */
> > + if (!crypto->key_pending)
> > + hp = quic_data(&hp_k, rx_hp_key, keylen);
> > + err = quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv, hp, crypto->version);
> > + if (err)
> > + goto out;
> > + err = crypto_aead_setauthsize(crypto->rx_tfm[phase], QUIC_TAG_LEN);
> > + if (err)
> > + goto out;
> > + err = crypto_aead_setkey(crypto->rx_tfm[phase], rx_key, keylen);
> > + if (err)
> > + goto out;
> > + if (hp) {
> > + err = crypto_skcipher_setkey(crypto->rx_hp_tfm, rx_hp_key, keylen);
> > + if (err)
> > + goto out;
> > + }
> > + pr_debug("%s: k: %16phN, iv: %12phN, hp_k:%16phN\n", __func__, k.data, iv.data, rx_hp_key);
> > +out:
> > + memzero_explicit(rx_key, sizeof(rx_key));
> > + memzero_explicit(rx_hp_key, sizeof(rx_hp_key));
> > + return err;
> > +}
>
> This isn't a bug, but these two functions are nearly identical.
>
> quic_crypto_tx_keys_derive_and_install() and
> quic_crypto_rx_keys_derive_and_install() share the same structure: derive
> keys via quic_crypto_keys_derive(), set AEAD auth size and key,
> conditionally set the HP skcipher key, pr_debug, and memzero_explicit the
> temporaries.
>
> The only differences are the member names accessed (tx_secret vs
> rx_secret, tx_iv vs rx_iv, tx_tfm vs rx_tfm, tx_hp_tfm vs rx_hp_tfm).
>
> Could these be unified into a single static helper that takes
> direction-specific pointers as parameters?
>
Looks possible, and I will give it a try.
Thanks.
next prev parent reply other threads:[~2026-03-04 21:58 UTC|newest]
Thread overview: 34+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-02-25 2:34 [PATCH net-next v10 00/15] net: introduce QUIC infrastructure and core subcomponents Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 01/15] net: define IPPROTO_QUIC and SOL_QUIC constants Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 02/15] net: build socket infrastructure for QUIC protocol Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 03/15] quic: provide common utilities and data structures Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 04/15] quic: provide family ops for address and protocol Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 05/15] quic: provide quic.h header files for kernel and userspace Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 06/15] quic: add stream management Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 07/15] quic: add connection id management Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 08/15] quic: add path management Xin Long
2026-03-03 8:22 ` Paolo Abeni
2026-03-04 21:25 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 09/15] quic: add congestion control Xin Long
2026-03-03 8:32 ` [net-next,v10,09/15] " Paolo Abeni
2026-03-04 21:41 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 10/15] quic: add packet number space Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 11/15] quic: add crypto key derivation and installation Xin Long
2026-03-03 8:32 ` [net-next,v10,11/15] " Paolo Abeni
2026-03-04 21:58 ` Xin Long [this message]
2026-02-25 2:34 ` [PATCH net-next v10 12/15] quic: add crypto packet encryption and decryption Xin Long
2026-03-03 8:32 ` [net-next,v10,12/15] " Paolo Abeni
2026-03-04 22:31 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 13/15] quic: add timer management Xin Long
2026-03-03 8:33 ` [net-next,v10,13/15] " Paolo Abeni
2026-03-04 23:03 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 14/15] quic: add packet builder base Xin Long
2026-03-03 8:33 ` [net-next,v10,14/15] " Paolo Abeni
2026-03-04 23:13 ` Xin Long
2026-03-03 9:18 ` [PATCH net-next v10 14/15] " Paolo Abeni
2026-03-04 23:26 ` Xin Long
2026-02-25 2:34 ` [PATCH net-next v10 15/15] quic: add packet parser base Xin Long
2026-03-03 8:33 ` [net-next,v10,15/15] " Paolo Abeni
2026-03-04 23:37 ` Xin Long
2026-03-03 9:16 ` [PATCH net-next v10 15/15] " Paolo Abeni
2026-03-05 0:14 ` Xin Long
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CADvbK_cWP-S1VdsfZJCc_iTxaeVnGTbrXrNsVdWTgYjdqLjwaA@mail.gmail.com \
--to=lucien.xin@gmail.com \
--cc=aahringo@redhat.com \
--cc=alibuda@linux.alibaba.com \
--cc=andrew.gospodarek@broadcom.com \
--cc=chuck.lever@oracle.com \
--cc=daniel@haxx.se \
--cc=davem@davemloft.net \
--cc=dhowells@redhat.com \
--cc=dreibh@simula.no \
--cc=edumazet@google.com \
--cc=hare@suse.de \
--cc=hepengtao@xiaomi.com \
--cc=horms@kernel.org \
--cc=illiliti@protonmail.com \
--cc=jbaron@akamai.com \
--cc=jlayton@kernel.org \
--cc=kernel-tls-handshake@lists.linux.dev \
--cc=kuba@kernel.org \
--cc=linkinjeon@kernel.org \
--cc=linux-cifs@vger.kernel.org \
--cc=mail@johnericson.me \
--cc=marc@fiuczynski.com \
--cc=marcelo.leitner@gmail.com \
--cc=matttbe@kernel.org \
--cc=mbuhl@openbsd.org \
--cc=metze@samba.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=pc@manguebit.com \
--cc=quic@lists.linux.dev \
--cc=sd@queasysnail.net \
--cc=smfrench@gmail.com \
--cc=steved@redhat.com \
--cc=tfanelli@redhat.com \
--cc=tom@talpey.com \
--cc=xiyou.wangcong@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox