Re: [net-next,v10,12/15] quic: add crypto packet encryption and decryption

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 packet encryption and decryption
>
> This patch adds core support for packet-level encryption and decryption
> using AEAD, including both payload protection and QUIC header protection.
> It introduces helpers to encrypt and decrypt packets for secure QUIC
> packet exchange and address validation.
>
> > diff --git a/net/quic/crypto.c b/net/quic/crypto.c
> > index a9de418e604e..a84fd8d7bf93 100644
> > --- a/net/quic/crypto.c
> > +++ b/net/quic/crypto.c
>
> [ ... ]
>
> > @@ -231,6 +562,137 @@ static struct quic_cipher ciphers[QUIC_CIPHER_MAX=
 + 1 - QUIC_CIPHER_MIN] =3D {
>
> [ ... ]
>
> > +/* Encrypts a QUIC packet before transmission.  This function performs=
 AEAD encryption of
> > + * the packet payload and applies header protection. It handles key ph=
ase tracking and key
> > + * update timing.
> > + *
> > + * Return: 0 on success, or a negative error code.
> > + */
> > +int quic_crypto_encrypt(struct quic_crypto *crypto, struct sk_buff *sk=
b)
> > +{
> > +     u8 *iv, cha, ccm, phase =3D crypto->key_phase;
> > +     struct quic_skb_cb *cb =3D QUIC_SKB_CB(skb);
> > +     int err;
> > +
> > +     cb->key_phase =3D phase;
> > +     iv =3D crypto->tx_iv[phase];
> > +     /* Packet payload is already encrypted (e.g., resumed from async)=
, proceed to header
> > +      * protection only.
> > +      */
> > +     if (cb->resume)
> > +             goto out;
> > +
> > +     /* If a key update is pending and this is the first packet using =
the new key, save the
> > +      * current time. Later used to clear old keys after some time has=
 passed (see
> > +      * quic_crypto_decrypt()).
> > +      */
> > +     if (crypto->key_pending && !crypto->key_update_send_time)
> > +             crypto->key_update_send_time =3D quic_ktime_get_us();
> > +
> > +     ccm =3D quic_crypto_is_cipher_ccm(crypto);
> > +     err =3D quic_crypto_payload_encrypt(crypto->tx_tfm[phase], skb, i=
v, ccm);
> > +     if (err)
> > +             return err;
> > +out:
> > +     cha =3D quic_crypto_is_cipher_chacha(crypto);
> > +     return quic_crypto_header_encrypt(crypto->tx_hp_tfm, skb, cha);
> > +}
>
> This function calls quic_crypto_payload_encrypt(). Looking at that
> function:
>
> > +/* AEAD Usage. */
> > +static int quic_crypto_payload_encrypt(struct crypto_aead *tfm, struct=
 sk_buff *skb,
> > +                                    u8 *tx_iv, bool ccm)
> > +{
> > +     struct quic_skb_cb *cb =3D QUIC_SKB_CB(skb);
> > +     u8 *iv, i, nonce[QUIC_IV_LEN];
> > +     struct aead_request *req;
> > +     struct sk_buff *trailer;
> > +     struct scatterlist *sg;
> > +     struct quichdr *hdr;
> > +     u32 nsg, hlen, len;
> > +     void *ctx;
> > +     __be64 n;
> > +     int err;
> > +
> > +     len =3D skb->len;
> > +     err =3D skb_cow_data(skb, QUIC_TAG_LEN, &trailer);
> > +     if (err < 0)
> > +             return err;
> > +     nsg =3D (u32)err;
> > +     pskb_put(skb, trailer, QUIC_TAG_LEN);
> > +     hdr =3D quic_hdr(skb);
> > +     hdr->key =3D cb->key_phase;
> > +
> > +     ctx =3D quic_crypto_aead_mem_alloc(tfm, 0, &iv, &req, &sg, nsg);
> > +     if (!ctx)
> > +             return -ENOMEM;
>
> If the kzalloc(GFP_ATOMIC) inside quic_crypto_aead_mem_alloc() fails,
> this returns -ENOMEM without rolling back the SKB modifications. The
> SKB is now permanently enlarged by QUIC_TAG_LEN (16 bytes) with a
> modified hdr->key.
>
> Compare with quic_crypto_payload_decrypt():
>
> > +static int quic_crypto_payload_decrypt(struct crypto_aead *tfm, struct=
 sk_buff *skb,
> > +                                    u8 *rx_iv, bool ccm)
> > +{
> > +     struct quic_skb_cb *cb =3D QUIC_SKB_CB(skb);
> > +     u8 *iv, i, nonce[QUIC_IV_LEN];
> > +     struct aead_request *req;
> > +     int nsg, hlen, len, err;
> > +     struct scatterlist *sg;
> > +     void *ctx;
> > +     __be64 n;
> > +
> > +     len =3D cb->length + cb->number_offset;
> > +     hlen =3D cb->number_offset + cb->number_len;
> > +     if (len - hlen < QUIC_TAG_LEN)
> > +             return -EINVAL;
> > +     nsg =3D 1; /* skb is already linearized in quic_packet_rcv(). */
> > +     ctx =3D quic_crypto_aead_mem_alloc(tfm, 0, &iv, &req, &sg, nsg);
> > +     if (!ctx)
> > +             return -ENOMEM;
> > +
> > +     sg_init_table(sg, nsg);
> > +     err =3D skb_to_sgvec(skb, sg, 0, len);
>
> In the decrypt path, quic_crypto_aead_mem_alloc() is called before any
> SKB modifications. Should quic_crypto_payload_encrypt() do the same to
> avoid leaving the SKB in a modified state if allocation fails?
>
This is not true.

- firstly, on RX or Decrypt path, skb_cow_data() is called in
quic_crypto_header_decrypt(), which is called before
quic_crypto_payload_decrypt() in quic_crypto_decrypt(), so
skb_cow_data() is called before mem_alloc for both places.

- secondly, even if the mem_alloc is failed, the skb will be dropped,
no issue could be caused.

> [ ... ]
>
> > +     /* Similar logic to quic_crypto_payload_encrypt(). */
> > +     memcpy(nonce, rx_iv, QUIC_IV_LEN);
> > +     n =3D cpu_to_be64(cb->number);
> > +     for (i =3D 0; i < sizeof(n); i++)
> > +             nonce[QUIC_IV_LEN - sizeof(n) + i] ^=3D ((u8 *)&n)[i];
>
> This isn't a bug, but there's duplicated logic between
> quic_crypto_payload_encrypt() and quic_crypto_payload_decrypt(). The
> AEAD nonce construction sequence (memcpy nonce from IV, XOR with be64
> packet number, set CCM byte) appears in both functions.
>
> In quic_crypto_payload_encrypt():
>
> > +     hlen =3D cb->number_offset + cb->number_len;
> > +     memcpy(nonce, tx_iv, QUIC_IV_LEN);
> > +     n =3D cpu_to_be64(cb->number);
> > +     for (i =3D 0; i < sizeof(n); i++)
> > +             nonce[QUIC_IV_LEN - sizeof(n) + i] ^=3D ((u8 *)&n)[i];
> > +
> > +     /* For CCM based ciphers, first byte of IV is a constant. */
> > +     iv[0] =3D TLS_AES_CCM_IV_B0_BYTE;
> > +     memcpy(&iv[ccm], nonce, QUIC_IV_LEN);
>
> And in quic_crypto_payload_decrypt():
>
> > +     /* Similar logic to quic_crypto_payload_encrypt(). */
> > +     memcpy(nonce, rx_iv, QUIC_IV_LEN);
> > +     n =3D cpu_to_be64(cb->number);
> > +     for (i =3D 0; i < sizeof(n); i++)
> > +             nonce[QUIC_IV_LEN - sizeof(n) + i] ^=3D ((u8 *)&n)[i];
> > +
> > +     iv[0] =3D TLS_AES_CCM_IV_B0_BYTE;
> > +     memcpy(&iv[ccm], nonce, QUIC_IV_LEN);
>
> The AEAD request teardown pattern (EINPROGRESS check, kfree_sensitive,
> memzero_explicit) is also duplicated. Would a shared static helper for
> nonce construction reduce the risk of the two copies diverging during
> maintenance?
>
I will see what I can do to dedup a bit for this.

Thanks.