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,12/15] quic: add crypto packet encryption and decryption
Date: Wed, 4 Mar 2026 17:31:57 -0500 [thread overview]
Message-ID: <CADvbK_fHhrCh-5zPDY1sh16TEKhJTWhZcUS4-pw=49krocV3SA@mail.gmail.com> (raw)
In-Reply-To: <20260303083255.35354-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 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] = {
>
> [ ... ]
>
> > +/* Encrypts a QUIC packet before transmission. This function performs AEAD encryption of
> > + * the packet payload and applies header protection. It handles key phase 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 *skb)
> > +{
> > + u8 *iv, cha, ccm, phase = crypto->key_phase;
> > + struct quic_skb_cb *cb = QUIC_SKB_CB(skb);
> > + int err;
> > +
> > + cb->key_phase = phase;
> > + iv = 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 = quic_ktime_get_us();
> > +
> > + ccm = quic_crypto_is_cipher_ccm(crypto);
> > + err = quic_crypto_payload_encrypt(crypto->tx_tfm[phase], skb, iv, ccm);
> > + if (err)
> > + return err;
> > +out:
> > + cha = 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 = 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 = skb->len;
> > + err = skb_cow_data(skb, QUIC_TAG_LEN, &trailer);
> > + if (err < 0)
> > + return err;
> > + nsg = (u32)err;
> > + pskb_put(skb, trailer, QUIC_TAG_LEN);
> > + hdr = quic_hdr(skb);
> > + hdr->key = cb->key_phase;
> > +
> > + ctx = 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 = 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 = cb->length + cb->number_offset;
> > + hlen = cb->number_offset + cb->number_len;
> > + if (len - hlen < QUIC_TAG_LEN)
> > + return -EINVAL;
> > + nsg = 1; /* skb is already linearized in quic_packet_rcv(). */
> > + ctx = quic_crypto_aead_mem_alloc(tfm, 0, &iv, &req, &sg, nsg);
> > + if (!ctx)
> > + return -ENOMEM;
> > +
> > + sg_init_table(sg, nsg);
> > + err = 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 = cpu_to_be64(cb->number);
> > + for (i = 0; i < sizeof(n); i++)
> > + nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((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 = cb->number_offset + cb->number_len;
> > + memcpy(nonce, tx_iv, QUIC_IV_LEN);
> > + n = cpu_to_be64(cb->number);
> > + for (i = 0; i < sizeof(n); i++)
> > + nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((u8 *)&n)[i];
> > +
> > + /* For CCM based ciphers, first byte of IV is a constant. */
> > + iv[0] = 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 = cpu_to_be64(cb->number);
> > + for (i = 0; i < sizeof(n); i++)
> > + nonce[QUIC_IV_LEN - sizeof(n) + i] ^= ((u8 *)&n)[i];
> > +
> > + iv[0] = 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.
next prev parent reply other threads:[~2026-03-04 22:32 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
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 [this message]
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_fHhrCh-5zPDY1sh16TEKhJTWhZcUS4-pw=49krocV3SA@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