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

[Sabrina Dubroca <sd@queasysnail.net>]

2025-11-13, 11:35:07 +0100, Ralf Lici wrote:
> On Wed, 2025-11-12 at 17:29 +0100, Sabrina Dubroca wrote:
> > 2025-11-11, 22:47:39 +0100, Antonio Quartulli wrote:
> > > +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.

IMO there's not much to gain here, since the rest of the code
(ovpn_aead_encrypt/decrypt) isn't ready for it. It may even be more
confusing since this bit looks generic but the rest isn't, and
figuring out why the packets are not being encrypted/decrypted
correctly could be a bit painful.

> 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.

Ah, good.

Then I would prefer to just make ovpn_aead_crypto_tmp_size always use
OVPN_NONCE_SIZE, and maybe add a comment in ovpn_aead_init referencing
ovpn_aead_crypto_tmp_size.

Something like:

/* basic AEAD assumption
 * all current algorithms use OVPN_NONCE_SIZE.
 * ovpn_aead_crypto_tmp_size and ovpn_aead_encrypt/decrypt
 * expect this.
 */


Or a

    DEBUG_NET_WARN_ON_ONCE(OVPN_NONCE_SIZE != crypto_aead_ivsize(tfm));

in ovpn_aead_crypto_tmp_size, which would fire if the check in
ovpn_aead_init is ever removed.


> > > @@ -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.

Thanks.

-- 
Sabrina