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

On Tue, Mar 3, 2026 at 3:33=E2=80=AFAM Paolo Abeni <[email protected]> wrot=
e:
>
> 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-loca=
l.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) +=3D quic.o
> >
> >  quic-y :=3D common.o family.o protocol.o socket.o stream.o connid.o pa=
th.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, struc=
t 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 =3D {};
> > +     struct quic_crypto_secret srt =3D {};
> > +     char *tl, *rl, *sal;
> > +     int err;
> > +
> > +     if (is_serv) {
> > +             rl =3D "client in";
> > +             tl =3D "server in";
> > +     } else {
> > +             tl =3D "client in";
> > +             rl =3D "server in";
> > +     }
> > +     sal =3D QUIC_INITIAL_SALT_V1;
> > +     if (version =3D=3D QUIC_VERSION_V2)
> > +             sal =3D 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 =3D 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 =3D quic_crypto_hkdf_extract(crypto->secret_tfm, ...); /* lin=
e 490 */
>         ...
>         err =3D quic_crypto_set_secret(crypto, &srt, version, 0);  /* lin=
e 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 initializati=
on:
>
> quic_crypto_set_secret() {
>         if (!crypto->cipher) {
>                 err =3D 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 =3D TLS_CIPHER_AES_GCM_128;
> > +     srt.send =3D 1;
> > +     err =3D quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &=
k);
> > +     if (err)
> > +             goto out;
> > +     err =3D 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 =3D TLS_CIPHER_AES_GCM_128;
> > +     srt.send =3D 0;
> > +     err =3D quic_crypto_hkdf_expand(crypto->secret_tfm, &s, &l, &z, &=
k);
> > +     if (err)
> > +             goto out;
> > +     err =3D 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 he=
ader protection key.
> > + */
> > +static int quic_crypto_tx_keys_derive_and_install(struct quic_crypto *=
crypto)
> > +{
> > +     struct quic_data srt =3D {}, k, iv, hp_k =3D {}, *hp =3D NULL;
> > +     u8 tx_key[QUIC_KEY_LEN], tx_hp_key[QUIC_KEY_LEN] =3D {};
> > +     int err, phase =3D crypto->key_phase;
> > +     u32 keylen, ivlen =3D QUIC_IV_LEN;
> > +
> > +     keylen =3D 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 =3D quic_data(&hp_k, tx_hp_key, keylen);
> > +     err =3D quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv=
, hp, crypto->version);
> > +     if (err)
> > +             goto out;
> > +     err =3D crypto_aead_setauthsize(crypto->tx_tfm[phase], QUIC_TAG_L=
EN);
> > +     if (err)
> > +             goto out;
> > +     err =3D crypto_aead_setkey(crypto->tx_tfm[phase], tx_key, keylen)=
;
> > +     if (err)
> > +             goto out;
> > +     if (hp) {
> > +             err =3D crypto_skcipher_setkey(crypto->tx_hp_tfm, tx_hp_k=
ey, 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 cu=
rrent key phase.
> > + * This installs AEAD decryption key, IV, and optionally header protec=
tion key.
> > + */
> > +static int quic_crypto_rx_keys_derive_and_install(struct quic_crypto *=
crypto)
> > +{
> > +     struct quic_data srt =3D {}, k, iv, hp_k =3D {}, *hp =3D NULL;
> > +     u8 rx_key[QUIC_KEY_LEN], rx_hp_key[QUIC_KEY_LEN] =3D {};
> > +     int err, phase =3D crypto->key_phase;
> > +     u32 keylen, ivlen =3D QUIC_IV_LEN;
> > +
> > +     keylen =3D 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 =3D quic_data(&hp_k, rx_hp_key, keylen);
> > +     err =3D quic_crypto_keys_derive(crypto->secret_tfm, &srt, &k, &iv=
, hp, crypto->version);
> > +     if (err)
> > +             goto out;
> > +     err =3D crypto_aead_setauthsize(crypto->rx_tfm[phase], QUIC_TAG_L=
EN);
> > +     if (err)
> > +             goto out;
> > +     err =3D crypto_aead_setkey(crypto->rx_tfm[phase], rx_key, keylen)=
;
> > +     if (err)
> > +             goto out;
> > +     if (hp) {
> > +             err =3D crypto_skcipher_setkey(crypto->rx_hp_tfm, rx_hp_k=
ey, 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.