Re: [PATCH v17 4/5] random: introduce generic vDSO getrandom() implementation
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: 2024-06-18 00:38:53
Also in:
linux-crypto, linux-patches, lkml
Subsystem:
generic vdso library, library code, the rest, x86 architecture (32-bit and 64-bit), x86 entry code, x86 vdso · Maintainers:
Andy Lutomirski, Thomas Gleixner, Vincenzo Frascino, Andrew Morton, Linus Torvalds, Ingo Molnar, Borislav Petkov, Dave Hansen
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:quoted
On Fri, Jun 14, 2024 at 12:08 PM Jason A. Donenfeld [off-list ref] wrote:quoted
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); }