Re: [PATCH v17 4/5] random: introduce generic vDSO getrandom() implementation

["Jason A. Donenfeld" <Jason@zx2c4.com>]

On Tue, Jun 18, 2024 at 02:12:40AM +0200, Jason A. Donenfeld 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@zx2c4.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?
> 
> If I've gotten you right this time, I'll add that argument as described.
> Seems straight forward to do. It's a bit annoying from a libc
> perspective, as the length has to be stored, but that's not impossible.

So, that looks like:

diff --git a/arch/x86/entry/vdso/vgetrandom.c b/arch/x86/entry/vdso/vgetrandom.c
index 6045ded5da90..794137fba649 100644
--- a/arch/x86/entry/vdso/vgetrandom.c
+++ b/arch/x86/entry/vdso/vgetrandom.c
@@ -6,12 +6,12 @@

 #include "../../../../lib/vdso/getrandom.c"

-ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags, void *state);
+ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len);

-ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags, void *state)
+ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len)
 {
-	return __cvdso_getrandom(buffer, len, flags, state);
+	return __cvdso_getrandom(buffer, len, flags, opaque_state, opaque_len);
 }

-ssize_t getrandom(void *, size_t, unsigned int, void *)
+ssize_t getrandom(void *, size_t, unsigned int, void *, size_t)
 	__attribute__((weak, alias("__vdso_getrandom")));
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 51251190a47e..4d89e34ff17d 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c
@@ -40,6 +40,7 @@ static void memcpy_and_zero_src(void *dst, void *src, size_t len)
  * @len:		Size of @buffer in bytes.
  * @flags:		Zero or more GRND_* flags.
  * @opaque_state:	Pointer to an opaque state area.
+ * @opaque_len:		Length of opaque state area, as returned by vgetrandom_alloc().
  *
  * This implements a "fast key erasure" RNG using ChaCha20, in the same way that the kernel's
  * getrandom() syscall does. It periodically reseeds its key from the kernel's RNG, at the same
@@ -55,7 +56,7 @@ static void memcpy_and_zero_src(void *dst, void *src, size_t len)
  */
 static __always_inline ssize_t
 __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
-		       unsigned int flags, void *opaque_state)
+		       unsigned int flags, void *opaque_state, size_t opaque_len)
 {
 	ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len);
 	struct vgetrandom_state *state = opaque_state;
@@ -69,6 +70,10 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 	if (unlikely(((unsigned long)opaque_state & ~PAGE_MASK) + sizeof(*state) > PAGE_SIZE))
 		return -EFAULT;

+	/* If the caller passes the wrong size, which might happen due to CRIU, fallback. */
+	if (unlikely(opaque_len != sizeof(*state)))
+		goto fallback_syscall;
+
 	/*
 	 * If the kernel's RNG is not yet ready, then it's not possible to provide random bytes from
 	 * userspace, because A) the various @flags require this to block, or not, depending on
@@ -222,7 +227,7 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_
 }

 static __always_inline ssize_t
-__cvdso_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state)
+__cvdso_getrandom(void *buffer, size_t len, unsigned int flags, void *opaque_state, size_t opaque_len)
 {
-	return __cvdso_getrandom_data(__arch_get_vdso_rng_data(), buffer, len, flags, opaque_state);
+	return __cvdso_getrandom_data(__arch_get_vdso_rng_data(), buffer, len, flags, opaque_state, opaque_len);
 }