netdev - Re: [PATCH net-next 6/8] ovpn: consolidate crypto allocations in one chunk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7315d47ac4cd7510ad9df7760e04c49bddd92383.camel@mandelbit.com>
Date: Thu, 13 Nov 2025 11:35:07 +0100
From: Ralf Lici <ralf@...delbit.com>
To: Sabrina Dubroca <sd@...asysnail.net>, Antonio Quartulli
	 <antonio@...nvpn.net>
Cc: netdev@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>, Jakub
 Kicinski	 <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 6/8] ovpn: consolidate crypto allocations in
 one chunk

On Wed, 2025-11-12 at 17:29 +0100, Sabrina Dubroca wrote:
> 2025-11-11, 22:47:39 +0100, Antonio Quartulli wrote:
> > diff --git a/drivers/net/ovpn/crypto_aead.c
> > b/drivers/net/ovpn/crypto_aead.c
> > index cb6cdf8ec317..9ace27fc130a 100644
> > --- a/drivers/net/ovpn/crypto_aead.c
> > +++ b/drivers/net/ovpn/crypto_aead.c
> > @@ -36,6 +36,105 @@ static int ovpn_aead_encap_overhead(const struct
> > ovpn_crypto_key_slot *ks)
> >  		crypto_aead_authsize(ks->encrypt);	/* Auth Tag
> > */
> >  }
> >  
> > +/*
> 
> nit: missing a 2nd * to make it kdoc?
> 

ACK.
> 

> > + * ovpn_aead_crypto_tmp_size - compute the size of a temporary
> > object containing
> > + *			       an AEAD request structure with extra
> > space for SG
> > + *			       and IV.
> > + * @tfm: the AEAD cipher handle
> > + * @nfrags: the number of fragments in the skb
> > + *
> > + * This function calculates the size of a contiguous memory block
> > that includes
> > + * the initialization vector (IV), the AEAD request, and an array
> > of scatterlist
> > + * entries. For alignment considerations, the IV is placed first,
> > followed by
> > + * the request, and then the scatterlist.
> > + * Additional alignment is applied according to the requirements of
> > the
> > + * underlying structures.
> > + *
> > + * Return: the size of the temporary memory that needs to be
> > allocated
> > + */
> > +static unsigned int ovpn_aead_crypto_tmp_size(struct crypto_aead
> > *tfm,
> > +					      const unsigned int
> > nfrags)
> > +{
> > +	unsigned int len = crypto_aead_ivsize(tfm);
> > +
> > +	if (likely(len)) {
> 
> Is that right?
> 
> Previously iv was reserved with a constant size (OVPN_NONCE_SIZE), and
> we're always going to write some data into ->iv via
> ovpn_pktid_aead_write, but now we're only reserving the crypto
> algorithm's IV size (which appear to be 12, ie OVPN_NONCE_SIZE, for
> both chachapoly and gcm(aes), so maybe it doesn't matter).

Exactly, I checked and both gcm-aes and chachapoly return an IV size
equal to OVPN_NONCE_SIZE, as you noted. I just thought it wouldn't hurt
to make the function a bit more generic in case we ever support
algorithms without an IV in the future, knowing that OVPN_NONCE_SIZE
matches ivsize for all current cases.

Also, there's a check in ovpn_aead_init to ensure that
crypto_aead_ivsize returns the expected value, so we're covered if
anything changes unexpectedly.

> > @@ -71,13 +171,15 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer,
> > struct ovpn_crypto_key_slot *ks,
> >  	if (unlikely(nfrags + 2 > (MAX_SKB_FRAGS + 2)))
> >  		return -ENOSPC;
> >  
> > -	/* sg may be required by async crypto */
> > -	ovpn_skb_cb(skb)->sg = kmalloc(sizeof(*ovpn_skb_cb(skb)-
> > >sg) *
> > -				       (nfrags + 2), GFP_ATOMIC);
> > -	if (unlikely(!ovpn_skb_cb(skb)->sg))
> > +	/* allocate temporary memory for iv, sg and req */
> > +	tmp = kmalloc(ovpn_aead_crypto_tmp_size(ks->encrypt,
> > nfrags),
> > +		      GFP_ATOMIC);
> > +	if (unlikely(!tmp))
> >  		return -ENOMEM;
> >  
> > -	sg = ovpn_skb_cb(skb)->sg;
> > +	iv = ovpn_aead_crypto_tmp_iv(ks->encrypt, tmp);
> > +	req = ovpn_aead_crypto_tmp_req(ks->encrypt, iv);
> > +	sg = ovpn_aead_crypto_req_sg(ks->encrypt, req);
> >  
> >  	/* sg table:
> >  	 * 0: op, wire nonce (AD,
> > len=OVPN_OP_SIZE_V2+OVPN_NONCE_WIRE_SIZE),
> > @@ -105,13 +207,6 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer,
> > struct ovpn_crypto_key_slot *ks,
> >  	if (unlikely(ret < 0))
> >  		return ret;
> >  
> > -	/* iv may be required by async crypto */
> > -	ovpn_skb_cb(skb)->iv = kmalloc(OVPN_NONCE_SIZE,
> > GFP_ATOMIC);
> > -	if (unlikely(!ovpn_skb_cb(skb)->iv))
> > -		return -ENOMEM;
> > -
> > -	iv = ovpn_skb_cb(skb)->iv;
> > -
> >  	/* concat 4 bytes packet id and 8 bytes nonce tail into 12
> > bytes
> >  	 * nonce
> >  	 */
> > @@ -130,11 +225,7 @@ int ovpn_aead_encrypt(struct ovpn_peer *peer,
> > struct ovpn_crypto_key_slot *ks,
> >  	/* AEAD Additional data */
> >  	sg_set_buf(sg, skb->data, OVPN_AAD_SIZE);
> >  
> > -	req = aead_request_alloc(ks->encrypt, GFP_ATOMIC);
> > -	if (unlikely(!req))
> > -		return -ENOMEM;
> > -
> > -	ovpn_skb_cb(skb)->req = req;
> > +	ovpn_skb_cb(skb)->crypto_tmp = tmp;
> 
> That should be done immediately after the allocation, so that any
> failure before this (skb_to_sgvec_nomark, ovpn_pktid_xmit_next) will
> not leak this blob? ovpn_aead_encrypt returns directly and lets
> ovpn_encrypt_post handle the error and free the memory, but only after
>  ->crypto_tmp has been set.
> 
> (same thing on the decrypt path)

Right, will fix both paths.

-- 
Ralf Lici
Mandelbit Srl