Message-ID: <Y4pEEy43LYlV35bh@zx2c4.com>
Date:   Fri, 2 Dec 2022 19:29:39 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Florian Weimer <fweimer@...hat.com>
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>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall

Hi Florian,

On Fri, Dec 02, 2022 at 06:17:17PM +0100, Florian Weimer wrote:
> * Jason A. Donenfeld:
> 
> > I don't think zapping that memory is supported, or even a sensible thing
> > to do. In the first place, I don't think we should suggest that the user
> > can dereference that pointer, at all. In that sense, maybe it's best to
> > call it a "handle" or something similar (a "HANDLE"! a "HWND"? a "HRNG"?
> 
> Surely the caller has to carve up the allocation, so the returned
> pointer is not opaque at all.  From Adhemerval's glibc patch:
> 
>       grnd_allocator.cap = new_cap;
>       grnd_allocator.states = new_states;
> 
>       for (size_t i = 0; i < num; ++i)
> 	{
> 	  grnd_allocator.states[i] = new_block;
> 	  new_block += size_per_each;
> 	}
>       grnd_allocator.len = num;
>     }
> 
> That's the opposite of a handle, really.

Right. (And the same code is in the commit message example too.)

> 
> >> But it will constrain future
> >> evolution of the implementation because you can't add registration
> >> (retaining a reference to the passed-in area in getrandom) after the
> >> fact.  But I'm not sure if this is possible with the current interface,
> >> either.  Userspace has to make some assumptions about the life-cycle to
> >> avoid a memory leak on thread exit.
> >
> > It sounds like this is sort of a different angle on Rasmus' earlier
> > comment about how munmap leaks implementation details. Maybe there's
> > something to that after all? Or not? I see two approaches:
> >
> > 1) Keep munmap as the allocation function. If later on we do fancy
> >    registration and in-kernel state tracking, or add fancy protection
> >    flags, or whatever else, munmap should be able to identify these
> >    pages and carry out whatever special treatment is necessary.
> 
> munmap is fine, but the interface needs to say how to use it, and what
> length to pass.

Glad we're on the same page. Indeed I've now documented this for my
in-progress v11. A blurb like:

+ * 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.
+ *
+ * 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().

What do you think of that text?

> > Then they're caught holding the bag? This doesn't seem much different
> > from userspace shooting themselves in general, like writing garbage into
> > the allocated states and then trying to use them. If this is something
> > you really, really are concerned about, then maybe my cheesy dumb xor
> > thing mentioned above would be a low effort mitigation here.
> 
> So the MAP_LOCKED is just there to prevent leakage to swap?

Right. I can combine that with MLOCK_ONFAULT and NORESERVED to avoid
having to commit the memory immediately. I've got this in my tree for
v11.

In case you're curious to see the WIP, it's in here:
https://git.zx2c4.com/linux-rng/log/?h=vdso

Jason

Powered by blists - more mailing lists