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
