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@xxxxxxxxx> 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);
}
