linux-kernel - Re: [PATCH v16 2/5] random: add vgetrandom

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: <ZmMcFonXPVtU0moO@zx2c4.com>
Date: Fri, 7 Jun 2024 16:41:26 +0200
From: "Jason A. Donenfeld" <Jason@...c4.com>
To: Eric Biggers <ebiggers@...nel.org>
Cc: linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
	tglx@...utronix.de, 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 2/5] random: add vgetrandom_alloc() syscall

On Tue, Jun 04, 2024 at 10:22:49AM -0700, Eric Biggers wrote:
> On Sat, Jun 01, 2024 at 12:56:40PM +0200, Jason A. Donenfeld wrote:
> > On Thu, May 30, 2024 at 08:59:17PM -0700, Eric Biggers wrote:
> > > On Tue, May 28, 2024 at 02:19:51PM +0200, Jason A. Donenfeld wrote:
> > > > +/**
> > > > + * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom().
> > > > + *
> > > > + * @num:	   On input, a pointer to a suggested hint of how many states to
> > > > + * 		   allocate, and on return the number of states actually allocated.
> > > > + *
> > > > + * @size_per_each: On input, must be zero. On return, the size of each state allocated,
> > > > + * 		   so that the caller can split up the returned allocation into
> > > > + * 		   individual states.
> > > > + *
> > > > + * @addr:	   Reserved, must be zero.
> > > > + *
> > > > + * @flags:	   Reserved, must be zero.
> > > > + *
> > > > + * The getrandom() vDSO function in userspace requires an opaque state, which
> > > > + * this function allocates by mapping a certain number of special pages into
> > > > + * the calling process. It takes a hint as to the number of opaque states
> > > > + * desired, and provides the caller with the number of opaque states actually
> > > > + * allocated, the size of each one in bytes, and the address of the first
> > > > + * state, which may be split up into @num states of @size_per_each bytes each,
> > > > + * by adding @size_per_each to the returned first state @num times, while
> > > > + * ensuring that no single state straddles a page boundary.
> > > > + *
> > > > + * Returns the address of the first state in the allocation on success, or a
> > > > + * negative error value on failure.
> > > > + *
> > > > + * The returned address of the first state may be passed to munmap(2) with a
> > > > + * length of `(size_t)num * (size_t)size_per_each`, in order to deallocate the
> > > > + * memory, after which it is invalid to pass it to vDSO getrandom().
> > > 
> > > Wouldn't a munmap with '(size_t)num * (size_t)size_per_each' be potentially too
> > > short, due to how the allocation is sized such that states don't cross page
> > > boundaries?
> > 
> > You're right, I think. The calculation should instead be something like:
> > 
> >     DIV_ROUND_UP(num, PAGE_SIZE / size_per_each) * PAGE_SIZE
> > 
> > Does that seem correct to you?
> > 
> 
> Yes, though I wonder if it would be better to give userspace the number of pages
> instead of the number of states.

Or maybe just the number of total bytes allocated? That would match
what's expected to be passed to munmap() and is maybe the easiest to
deal with. I'll give that a shot for v+1.

Jason