Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: 2022-12-01 02:16:42
Also in:
linux-crypto, linux-patches, lkml
Subsystem:
generic vdso library, library code, random number driver, the rest · Maintainers:
Andy Lutomirski, Thomas Gleixner, Vincenzo Frascino, Andrew Morton, "Theodore Ts'o", Jason A. Donenfeld, Linus Torvalds
Possibly related (same subject, not in this thread)
- 2022-12-02 · Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall · "Jason A. Donenfeld" <Jason@zx2c4.com>
- 2022-12-02 · Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall · Florian Weimer <hidden>
- 2022-12-01 · Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall · "Jason A. Donenfeld" <Jason@zx2c4.com>
- 2022-11-30 · RE: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall · David Laight <hidden>
- 2022-11-30 · Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall · "Jason A. Donenfeld" <Jason@zx2c4.com>
On Wed, Nov 30, 2022 at 04:39:55PM +0100, Jason A. Donenfeld wrote:
quoted
Can userspace use the memory for something else if it's not passed to getrandom?I suspect the documentation answer here is, "no", even if technically it might happen to work on this kernel or that kernel. I suppose this could even be quasi-enforced by xoring the top bits with some vdso compile-time constant, so you can't rely on being able to dereference it yourself. [...] 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.
I implemented a sample of this, below. I think this is a bit silly, though, and making this fully robust could take some effort. Overall, I don't think we should do this. However, the more I think about the args thing from the last email, the more I like *that* idea. So I think I'll roll with that. But this cheesy pointer obfuscation thing here, meh. But here's what it could look like anyway:
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 2aaeb48d11be..7aff45165ce5 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c@@ -228,7 +228,7 @@ SYSCALL_DEFINE2(vgetrandom_alloc, struct vgetrandom_alloc_args __user *, uargs, if (args.flags & VGRA_DEALLOCATE) { if (args.size_per_each != state_size || args.num > max_states || !args.states) return -EINVAL; - return vm_munmap(args.states, args.num * state_size); + return vm_munmap(args.states ^ VGETRANDOM_STATE_HI_TAINT, args.num * state_size); } /* These don't make sense as input values if allocating, so reject them. */
@@ -249,7 +249,7 @@ SYSCALL_DEFINE2(vgetrandom_alloc, struct vgetrandom_alloc_args __user *, uargs, args.num = num_states; args.size_per_each = state_size; - args.states = pages_addr; + args.states = pages_addr ^ VGETRANDOM_STATE_HI_TAINT; ret = -EFAULT; if (copy_to_user(uargs, &args, sizeof(args)))
diff --git a/include/vdso/getrandom.h b/include/vdso/getrandom.h
index cb624799a8e7..9a6aaf4d99d4 100644
--- a/include/vdso/getrandom.h
+++ b/include/vdso/getrandom.h@@ -8,6 +8,7 @@ #include <crypto/chacha.h> #include <vdso/limits.h> +#include <linux/version.h> /** * struct vgetrandom_state - State used by vDSO getrandom() and allocated by vgetrandom_alloc().
@@ -41,4 +42,10 @@ struct vgetrandom_state { bool in_use; }; +/* Be annoying by changing frequently enough. */ +#define VGETRANDOM_STATE_HI_TAINT ((unsigned long)(((LINUX_VERSION_CODE >> 16) + \ + (LINUX_VERSION_CODE >> 8) + (LINUX_VERSION_CODE >> 0) + \ + __GNUC__ + __GNUC_MINOR__ + __GNUC_PATCHLEVEL__) \ + & 0xff) << (BITS_PER_LONG - 8)) + #endif /* _VDSO_GETRANDOM_H */
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
index 9ca624756432..14cbd349186c 100644
--- a/lib/vdso/getrandom.c
+++ b/lib/vdso/getrandom.c@@ -57,7 +57,7 @@ __cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_ unsigned int flags, void *opaque_state) { ssize_t ret = min_t(size_t, INT_MAX & PAGE_MASK /* = MAX_RW_COUNT */, len); - struct vgetrandom_state *state = opaque_state; + struct vgetrandom_state *state = (void *)((unsigned long)opaque_state ^ VGETRANDOM_STATE_HI_TAINT); size_t batch_len, nblocks, orig_len = len; unsigned long current_generation; void *orig_buffer = buffer;