linux-kernel - RE: [PATCH v17 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] [day] [month] [year] [list]

Date: Wed, 19 Jun 2024 11:36:46 +0000
From: David Laight <David.Laight@...LAB.COM>
To: "'Jason A. Donenfeld'" <Jason@...c4.com>, Andy Lutomirski
	<luto@...capital.net>
CC: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"patches@...ts.linux.dev" <patches@...ts.linux.dev>, "tglx@...utronix.de"
	<tglx@...utronix.de>, "linux-crypto@...r.kernel.org"
	<linux-crypto@...r.kernel.org>, "linux-api@...r.kernel.org"
	<linux-api@...r.kernel.org>, "x86@...nel.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 v17 4/5] random: introduce generic vDSO getrandom()
 implementation

From: Jason A. Donenfeld
> Sent: 18 June 2024 20:28
> On Tue, Jun 18, 2024 at 10:55:17AM -0700, Andy Lutomirski wrote:
> > On Mon, Jun 17, 2024 at 5:12 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > >
> > > Hi Andy,
> > >
> > > On Mon, Jun 17, 2024 at 05:06:22PM -0700, Andy Lutomirski wrote:
> > > > On Fri, Jun 14, 2024 at 12:08 PM Jason A. Donenfeld <Jason@...c4.com> wrote:
> > > > >
> > > > > Provide a generic C vDSO getrandom() implementation, which operates on
> > > > > an opaque state returned by vgetrandom_alloc() and produces random bytes
> > > > > the same way as getrandom(). This has a the API signature:
> > > > >
> > > > >   ssize_t vgetrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state);
> > > >
> > > > Last time around, I mentioned some potential issues with this function
> > > > signature, and I didn't see any answer.  My specific objection was to
> > > > the fact that the caller passes in a pointer but not a length, and
> > > > this potentially makes reasoning about memory safety awkward,
> > > > especially if anything like CRIU is involved.
> > >
> > > Oh, I understood this backwards last time - I thought you were
> > > criticizing the size_t len argument, which didn't make any sense.
> > >
> > > Re-reading now, what you're suggesting is that I add an additional
> > > argument called `size_t opaque_len`, and then the implementation does
> > > something like:
> > >
> > >     if (opaque_len != sizeof(struct vgetrandom_state))
> > >         goto fallback_syscall;
> > >
> > > With the reasoning that falling back to syscall is better than returning
> > > -EINVAL, because that could happen in a natural way due to CRIU. In
> > > contrast, your objection to opaque_state not being aligned falling back
> > > to the syscall was that it should never happen ever, so -EFAULT is more
> > > fitting.
> > >
> > > Is that correct?
> >
> > My alternative suggestion, which is far less well formed, would be to
> > make the opaque argument be somehow not pointer-like and be more of an
> > opaque handle.  So it would be uintptr_t instead of void *, and the
> > user API would be built around the user getting a list of handles
> > instead of a block of memory.
> >
> > The benefit would be a tiny bit less overhead (potentially), but the
> > API would need substantially more rework.  I'm not convinced that this
> > would be worthwhile.
> 
> I'd thought about this too -- a Windows-style handle system -- but
> it seemed complicated and just not worth it, so the simplicity here
> seems more appealing. I'm happy to take your suggestion of an opaque_len
> argument (and it's already implemented in my "vdso" branch), and
> leaving it at that.

They don't work either...

Probably best is to make it 'struct vgetrandom_state *' but never
actually define that structure in any user header.
Then at least the application gets some type checking from the compiler
that the correct pointer is being passed.

Depending on where/how the data is allocated you may then not need
a length? 

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)