linux-kernel - Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom() implementation

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]

Date: Fri, 7 Jun 2024 17:59:42 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
	linux-crypto@...r.kernel.org, linux-api@...r.kernel.org,
	x86@...nel.org, Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>,
	Carlos O'Donell <carlos@...hat.com>,
	Florian Weimer <fweimer@...hat.com>, Arnd Bergmann <arnd@...db.de>,
	Jann Horn <jannh@...gle.com>,
	Christian Brauner <brauner@...nel.org>,
	David Hildenbrand <dhildenb@...hat.com>
Subject: Re: [PATCH v16 4/5] random: introduce generic vDSO getrandom()
 implementation

On Thu, Jun 06, 2024 at 12:10:00AM +0200, Thomas Gleixner wrote:
> Jason!
> 
> On Wed, Jun 05 2024 at 23:03, Thomas Gleixner wrote:
> > On Tue, May 28 2024 at 14:19, Jason A. Donenfeld wrote:
> >> + */
> >> +#ifdef CONFIG_64BIT
> >> +typedef u64 vdso_kernel_ulong;
> >> +#else
> >> +typedef u32 vdso_kernel_ulong;
> >> +#endif
> >
> > All of this is pointless because if a 32-bit application runs on a
> > 64-bit kernel it has to use the 64-bit 'generation'. So why on earth do
> > we need magic here for a 32-bit kernel?
> >
> > Just use u64 for both and spare all this voodoo. We're seriously not
> > "optimizing" for 32-bit kernels.
> 
> All what happens on a 32-bit kernel is that the RNG will store the
> unsigned long (32bit) generation into a 64bit variable:
> 
> 	smp_store_release(&_vdso_rng_data.generation, next_gen + 1);
> 
> As the upper 32bit are always zero, there is no issue vs. load store
> tearing at all. So there is zero benefit for this aside of slightly
> "better" user space code when running on a 32-bit kernel. Who cares?

Oh yea. Okay, great. I was concerned about the tearing, but I guess it's
really a non issue. So I'll just make it a u64 and all of this
complexity can just go away. Thanks for thinking about it in a less
convoluted way than me.

> While staring at this I wonder where the corresponding
> smp_load_acquire() is. I haven't found one in the VDSO code.
> READ_ONCE() is only equivalent on a few architectures.
> 
> But, what does that store_release() buy at all? There is zero ordering
> vs. anything in the kernel and neither against user space.
> 
> If that smp_store_release() serves a purpose then it really has to be
> extensively documented especially as the kernel itself simply uses
> WRITE/READ_ONCE() for base_rng.generation.

This came up here too: https://lore.kernel.org/all/Y3l6ocn1dTN0+1GK@zx2c4.com/

It's to order the writes to the generation counter and is_ready.

Jason