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: <34c250472e36c5b506aed9416420d608a9b3969b.camel@mandelbit.com>
Date: Thu, 13 Nov 2025 17:32:42 +0100
From: Ralf Lici <ralf@...delbit.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: Antonio Quartulli <antonio@...nvpn.net>, 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 Thu, 2025-11-13 at 14:48 +0100, Sabrina Dubroca wrote:
> 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.

That’s a good point, actually.

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

I'll go with both to clearly assert the assumption.

Thanks,

-- 
Ralf Lici
Mandelbit Srl