Hi,
Here's my humble hack at improving kernel for a faster secure arc4random()
userspace implementation, by allowing userspace to buffer getrandom()
generated entropy, discarding it as the kernel's own CSPRNG is reseeded.
It's largely built upon the vDSO work of Jason A. Donenfeld, as part of
its latest patchset "[PATCH v14 0/7] implement getrandom() in vDSO" [1]
but it's made simpler by making available only one of the missing tools
for the userspace to properly buffer the output of getrandom().
Using MADV_WIPEONFORK and mlock(), userspace can reasonably offer forward
secrecy*, until something like VM_DROPPABLE[2] is provided by the kernel,
to allow for the buffer memory to never, ever be written to the disk
before its used, being inherited accross fork(), and isn't limited by
RLIMIT_MEMLOCK.
* provided userspace can mlock() the memory and calls mlock() on buffer
after fork, as memory locks are not inherited accross fork().
As it's a hack, it's far from perfect. The main drawback I see is the
case where fresh entropy has to be discarded as the kernel's CSPRNG
generation is updated as the result of calling getrandom() to generate
the mentionned entropy. The workaround, is to limit the amount of fresh
entropy fetched when a kernel's CSPRNG generation change is detected,
and to increase the amount the data retrieved with getrandom() when
generation doesn't change between calls.
Performance wise, the improvements are here, as one can check with the
test program provided:
getrandom(,,GRND_TIMESTAMP) test
getrandom() support GRND_TIMESTAMP
found getrandom() in vDSO at 0x7ffc3efccc60
== direct syscall getrandom(), 16777216 u32, 2.866324020 s, 5.853 M u32/s, 170.846 ns/u32
== direct vDSO getrandom(), 16777216 u32, 2.883473280 s, 5.818 M u32/s, 171.868 ns/u32
== pooled syscall getrandom(), 16777216 u32, 1.152421219 s, 14.558 M u32/s, 68.690 ns/u32, (0 bytes discarded)
== pooled vDSO getrandom(), 16777216 u32, 0.162477863 s, 103.258 M u32/s, 9.684 ns/u32, (0 bytes discarded)
With the requirement to mlock() the memory page(s) used to buffer
getrandom() output, I'm not sure userspace could afford to allocate
4KBytes per thread, before being hit by RLIMIT_MEMLOCK (or worse,
OOM killer). Thus, some form of sharing between threads would be
needed, which would require locking, reducing the performances
shown above.
Also I haven't studied the security impact of making the kernel base
CSPRNG seed generation available to userspace. It can be made more
opaque if needed.
Regards.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]/
Jason A. Donenfeld (2):
random: introduce generic vDSO getrandom(,, GRND_TIMESTAMP) fast path
x86: vdso: Wire up getrandom() vDSO implementation.
Yann Droneaud (2):
random: introduce getrandom() GRND_TIMESTAMP
testing: add a getrandom() GRND_TIMESTAMP vDSO demonstration/benchmark
MAINTAINERS | 1 +
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/entry/vdso/vdso.lds.S | 2 +
arch/x86/entry/vdso/vgetrandom.c | 17 +
arch/x86/include/asm/vdso/getrandom.h | 42 +++
arch/x86/include/asm/vdso/vsyscall.h | 2 +
arch/x86/include/asm/vvar.h | 16 +
drivers/char/random.c | 52 ++-
include/linux/random.h | 31 ++
include/uapi/linux/random.h | 2 +
include/vdso/datapage.h | 9 +
lib/vdso/Kconfig | 5 +
lib/vdso/getrandom.c | 51 +++
tools/testing/crypto/getrandom/Makefile | 4 +
.../testing/crypto/getrandom/test-getrandom.c | 307 ++++++++++++++++++
16 files changed, 543 insertions(+), 2 deletions(-)
create mode 100644 arch/x86/entry/vdso/vgetrandom.c
create mode 100644 arch/x86/include/asm/vdso/getrandom.h
create mode 100644 lib/vdso/getrandom.c
create mode 100644 tools/testing/crypto/getrandom/Makefile
create mode 100644 tools/testing/crypto/getrandom/test-getrandom.c
--
2.37.2
In the pursuit of implementing an userspace arc4random() fast
enough to be used in place of rand(), random(), lrand48(),
mrand48(), etc., and as strong as getrandom(), it was found
that calling getrandom() to generate one uint32_t at a time
is not fast enough (see [1] for example).
As noted by Florian Weimer in [2]:
"The performance numbers suggest that we benefit from buffering in user
space. It might not be necessary to implement expansion in userspace.
getrandom (or /dev/urandom) with a moderately-sized buffer could be
sufficient."
Generating multiple values ahead of time with a proper CSPRNG
helps achieve better performances. But buffering in userspace
come with a lot of security related hurdles and pitfalls.
As noted by Jason A. Donenfeld in [3]
"For example, the kernel reseeds itself when virtual machines fork using
an identifier passed to the kernel via ACPI. It also reseeds itself on
system resume, both from ordinary S3 sleep but also, more importantly,
from hibernation."
Ignoring for now the issue of securily store the buffered
random values in memory to achieve forward secrecy [4], it's
possible to devise a mechanism to help userspace to know
when to discard the values generated/buffered from a previous
call to getrandom() so that the VM and/or resume issue can
be dealt with at userspace level.
Instead of adding a new system call, this patch shoehorns a
query mechanism in getrandom() syscall by adding a mean to
get and test a "timestamp". Currently, the "timestamp" is a
single 64bit integer, that maps to the kernel's base CSPRNG
generation, inverted, so that 0 means unintialized.
GRND_TIMESTAMP allows userspace to ask the kernel if previous
"timestamp" has changed as the result of an event that
triggered kernel CSPRNG reseed, and to update the "timestamp".
In case the "timestamp" hasn't changed, userspace CSPRNG can
consume a slice of its buffered random stream.
If it has changed, remaining userspace buffered random values
should be discarded. Userspace should call getrandom() to fill
and/or generate its buffer with updated seed.
It's advised to test again the "timestamp" to deal with the
race condition, where the kernel reseed just after the call
to getrandom() to get entropy.
How to not use it (because it doesn't have reseed on fork(),
aka. MADV_WIPEONFORK, and forward secrecy buffer protection
aka. mlock(), see [4]):
static bool expired(void)
{
static uint64_t grnd_ts;
ret = getrandom(&grnd_ts, sizeof(grnd_ts), GRND_TIMESTAMP);
if (ret < 0)
abort(); /* TODO: proper fallback to unbuffered getrandom() */
return !!ret;
}
uint32_t arc4random(void)
{
static uint32_t buffer[128]; /* TODO: mlock() buffer memory */
static unsigned int avail;
uint32_t val;
while (expired() || !avail) {
getrandom(buffer, sizeof(buffer), 0);
avail = 128;
}
avail--;
val = buffer[avail];
buffer[avail] = 0;
return val;
}
As the "timestamp" query has to be made for each uint32_t value
generated by arc4random(), the query should be as lightweight
as possible, thus it's expected GRND_TIMESTAMP to be handled by
at the vDSO level to prevent a system call.
[1] https://lore.kernel.org/all/[email protected]/
[2] https://sourceware.org/pipermail/libc-alpha/2022-July/140963.html
[3] https://sourceware.org/pipermail/libc-alpha/2022-July/140939.html
[4] https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Cc: [email protected]
Cc: Theodore Ts'o <[email protected]>
Cc: Adhemerval Zanella Netto <[email protected]>
Cc: Florian Weimer <[email protected]>
Cc: Jason A. Donenfeld <[email protected]>
Signed-off-by: Yann Droneaud <[email protected]>
---
drivers/char/random.c | 46 ++++++++++++++++++++++++++++++++++++-
include/linux/random.h | 31 +++++++++++++++++++++++++
include/uapi/linux/random.h | 2 ++
3 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/drivers/char/random.c b/drivers/char/random.c
index ce3ccd172cc8..9e2a37e432c0 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -1361,15 +1361,59 @@ static void __cold try_to_generate_entropy(void)
*
**********************************************************************/
+static ssize_t get_random_timestamp(char __user *ubuf, size_t len, unsigned int flags)
+{
+ u64 ts;
+
+ /* other combination not supported */
+ if (WARN(flags != GRND_TIMESTAMP, "GRND_TIMESTAMP cannot be used with other flags"))
+ return -EINVAL;
+
+ /* shorter structure not supported */
+ if (len < sizeof(ts))
+ return -EINVAL;
+
+ if (copy_from_user(&ts, ubuf, sizeof(ts)))
+ return -EFAULT;
+
+ /* longer structure supported, only if 0-padded,
+ timestamp might be extended in the future with more fields */
+ if (len > sizeof(ts)) {
+ char __user *p = ubuf + sizeof(ts);
+ size_t l = len - sizeof(ts);
+
+ while (l) {
+ char b;
+
+ if (get_user(b, p++))
+ return -EFAULT;
+
+ if (b)
+ return -EINVAL;
+ }
+ }
+
+ if (!get_random_timestamp_update(&ts, READ_ONCE(base_crng.generation)))
+ return 0;
+
+ if (copy_to_user(ubuf, &ts, sizeof(ts)))
+ return -EFAULT;
+
+ return sizeof(ts);
+}
+
SYSCALL_DEFINE3(getrandom, char __user *, ubuf, size_t, len, unsigned int, flags)
{
struct iov_iter iter;
struct iovec iov;
int ret;
- if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE))
+ if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE | GRND_TIMESTAMP))
return -EINVAL;
+ if (unlikely(flags & GRND_TIMESTAMP))
+ return get_random_timestamp(ubuf, len, flags);
+
/*
* Requesting insecure and blocking randomness at the same time makes
* no sense.
diff --git a/include/linux/random.h b/include/linux/random.h
index b0a940af4fff..bc219b5a96a5 100644
--- a/include/linux/random.h
+++ b/include/linux/random.h
@@ -161,4 +161,35 @@ int random_online_cpu(unsigned int cpu);
extern const struct file_operations random_fops, urandom_fops;
#endif
+/*
+ * get_random_timestamp_update()
+ *
+ * @generation: current CRNG generation (from base_crng.generation
+ * or _vdso_rng_data.generation)
+ *
+ * Return: timestamp size if previous timestamp is expired and is updated,
+ * 0 if not expired (and not updated)
+ */
+static inline bool get_random_timestamp_update(u64 *user_ts,
+ u64 generation)
+{
+ u64 ts;
+
+ /* base_crng.generation is never ULONG_MAX,
+ * OTOH userspace will initialize its timestamp
+ * to 0, so inverting base_crng.generation ensure
+ * first call to getrandom(,,GRND_TIMESTAMP) will
+ * update
+ */
+ ts = ~generation;
+
+ /* not expired ? no refresh suggested */
+ if (*user_ts == ts)
+ return false;
+
+ *user_ts = ts;
+
+ return true;
+}
+
#endif /* _LINUX_RANDOM_H */
diff --git a/include/uapi/linux/random.h b/include/uapi/linux/random.h
index e744c23582eb..b433fb8d79ac 100644
--- a/include/uapi/linux/random.h
+++ b/include/uapi/linux/random.h
@@ -50,9 +50,11 @@ struct rand_pool_info {
* GRND_NONBLOCK Don't block and return EAGAIN instead
* GRND_RANDOM No effect
* GRND_INSECURE Return non-cryptographic random bytes
+ * GRND_TIMESTAMP Interpret buffer as an opaque timestamp structure
*/
#define GRND_NONBLOCK 0x0001
#define GRND_RANDOM 0x0002
#define GRND_INSECURE 0x0004
+#define GRND_TIMESTAMP 0x0008
#endif /* _UAPI_LINUX_RANDOM_H */
--
2.37.2
From: "Jason A. Donenfeld" <[email protected]>
Exports base_crng.generation to vDSO and adds getrandom() to the vDSO.
Based on Jason A. Donenfeld <[email protected]> patch [1]
"[PATCH v14 6/7] random: introduce generic vDSO getrandom() implementation",
but deal only with GRND_TIMESTAMP in vDSO: generating random stream
is left to the getrandom() syscall.
[1] https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Yann Droneaud <[email protected]>
---
MAINTAINERS | 1 +
drivers/char/random.c | 6 +++++
include/vdso/datapage.h | 9 ++++++++
lib/vdso/Kconfig | 5 ++++
lib/vdso/getrandom.c | 51 +++++++++++++++++++++++++++++++++++++++++
5 files changed, 72 insertions(+)
create mode 100644 lib/vdso/getrandom.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 7f86d02cb427..20e1fabcb2e9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -17521,6 +17521,7 @@ T: git https://git.kernel.org/pub/scm/linux/kernel/git/crng/random.git
S: Maintained
F: drivers/char/random.c
F: drivers/virt/vmgenid.c
+F: lib/vdso/getrandom.c
RAPIDIO SUBSYSTEM
M: Matt Porter <[email protected]>
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 9e2a37e432c0..a60f50c95ab1 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c
@@ -56,6 +56,9 @@
#include <linux/sched/isolation.h>
#include <crypto/chacha.h>
#include <crypto/blake2s.h>
+#ifdef CONFIG_VDSO_GETRANDOM
+#include <vdso/datapage.h>
+#endif
#include <asm/archrandom.h>
#include <asm/processor.h>
#include <asm/irq.h>
@@ -271,6 +274,9 @@ static void crng_reseed(struct work_struct *work)
if (next_gen == ULONG_MAX)
++next_gen;
WRITE_ONCE(base_crng.generation, next_gen);
+#ifdef CONFIG_VDSO_GETRANDOM
+ smp_store_release(&_vdso_rng_data.generation, next_gen);
+#endif
if (!static_branch_likely(&crng_is_ready))
crng_init = CRNG_READY;
spin_unlock_irqrestore(&base_crng.lock, flags);
diff --git a/include/vdso/datapage.h b/include/vdso/datapage.h
index 73eb622e7663..7ae8e7ffe3ba 100644
--- a/include/vdso/datapage.h
+++ b/include/vdso/datapage.h
@@ -109,6 +109,14 @@ struct vdso_data {
struct arch_vdso_data arch_data;
};
+/**
+ * struct vdso_rng_data - vdso RNG state information
+ * @generation: counter representing the number of RNG reseeds
+ */
+struct vdso_rng_data {
+ u64 generation;
+};
+
/*
* We use the hidden visibility to prevent the compiler from generating a GOT
* relocation. Not only is going through a GOT useless (the entry couldn't and
@@ -120,6 +128,7 @@ struct vdso_data {
*/
extern struct vdso_data _vdso_data[CS_BASES] __attribute__((visibility("hidden")));
extern struct vdso_data _timens_data[CS_BASES] __attribute__((visibility("hidden")));
+extern struct vdso_rng_data _vdso_rng_data __attribute__((visibility("hidden")));
/*
* The generic vDSO implementation requires that gettimeofday.h
diff --git a/lib/vdso/Kconfig b/lib/vdso/Kconfig
index d883ac299508..3b394fa83f65 100644
--- a/lib/vdso/Kconfig
+++ b/lib/vdso/Kconfig
@@ -31,3 +31,8 @@ config GENERIC_VDSO_TIME_NS
VDSO
endif
+
+config VDSO_GETRANDOM
+ bool
+ help
+ Selected by architectures that support vDSO getrandom().
diff --git a/lib/vdso/getrandom.c b/lib/vdso/getrandom.c
new file mode 100644
index 000000000000..827351a87002
--- /dev/null
+++ b/lib/vdso/getrandom.c
@@ -0,0 +1,51 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Jason A. Donenfeld <[email protected]>. All Rights Reserved.
+ */
+
+#include <linux/cache.h>
+#include <linux/kernel.h>
+#include <linux/time64.h>
+#include <vdso/datapage.h>
+#include <asm/vdso/getrandom.h>
+#include <asm/vdso/vsyscall.h>
+
+/**
+ * __cvdso_getrandom_data - Generic vDSO implementation of getrandom() syscall.
+ * @rng_info: Describes state of kernel RNG, memory shared with kernel.
+ * @buffer: Input/Output buffer.
+ * @len: Size of @buffer in bytes.
+ * @flags: Zero or more GRND_* flags.
+ */
+static __always_inline ssize_t
+__cvdso_getrandom_data(const struct vdso_rng_data *rng_info, void *buffer, size_t len,
+ unsigned int flags)
+{
+ if (flags != GRND_TIMESTAMP)
+ goto fallback;
+
+ if (unlikely(!buffer))
+ goto fallback;
+
+ /* want aligned access */
+ if (unlikely(!IS_ALIGNED((uintptr_t)buffer, __alignof__(u64))))
+ goto fallback;
+
+ if (unlikely(len != sizeof(u64)))
+ goto fallback;
+
+ if (!get_random_timestamp_update((u64 *)buffer,
+ READ_ONCE(rng_info->generation)))
+ return 0;
+
+ return sizeof(u64);
+
+fallback:
+ return getrandom_syscall(buffer, len, flags);
+}
+
+static __always_inline ssize_t
+__cvdso_getrandom(void *buffer, size_t len, unsigned int flags)
+{
+ return __cvdso_getrandom_data(__arch_get_vdso_rng_data(), buffer, len, flags);
+}
--
2.37.2
From: "Jason A. Donenfeld" <[email protected]>
Hook up the generic vDSO implementation to the x86 vDSO data page.
Since the existing vDSO infrastructure is heavily based on the
timekeeping functionality, which works over arrays of bases,
a new macro is introduced for vvars that are not arrays.
Based on Jason A. Donenfeld patch [1]
"[PATCH v14 7/7] x86: vdso: Wire up getrandom() vDSO implementation"
removing the ChaCha20 implementation and opaque state argument
from vDSO getrandom().
[1] https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Yann Droneaud <[email protected]>
---
arch/x86/Kconfig | 1 +
arch/x86/entry/vdso/Makefile | 3 +-
arch/x86/entry/vdso/vdso.lds.S | 2 ++
arch/x86/entry/vdso/vgetrandom.c | 17 +++++++++++
arch/x86/include/asm/vdso/getrandom.h | 42 +++++++++++++++++++++++++++
arch/x86/include/asm/vdso/vsyscall.h | 2 ++
arch/x86/include/asm/vvar.h | 16 ++++++++++
7 files changed, 82 insertions(+), 1 deletion(-)
create mode 100644 arch/x86/entry/vdso/vgetrandom.c
create mode 100644 arch/x86/include/asm/vdso/getrandom.h
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 3604074a878b..df48387f019f 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -272,6 +272,7 @@ config X86
select HAVE_UNSTABLE_SCHED_CLOCK
select HAVE_USER_RETURN_NOTIFIER
select HAVE_GENERIC_VDSO
+ select VDSO_GETRANDOM
select HOTPLUG_SMT if SMP
select IRQ_FORCED_THREADING
select NEED_PER_CPU_EMBED_FIRST_CHUNK
diff --git a/arch/x86/entry/vdso/Makefile b/arch/x86/entry/vdso/Makefile
index 838613ac15b8..2565c4702f54 100644
--- a/arch/x86/entry/vdso/Makefile
+++ b/arch/x86/entry/vdso/Makefile
@@ -27,7 +27,7 @@ VDSO32-$(CONFIG_X86_32) := y
VDSO32-$(CONFIG_IA32_EMULATION) := y
# files to link into the vdso
-vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o
+vobjs-y := vdso-note.o vclock_gettime.o vgetcpu.o vgetrandom.o
vobjs32-y := vdso32/note.o vdso32/system_call.o vdso32/sigreturn.o
vobjs32-y += vdso32/vclock_gettime.o
vobjs-$(CONFIG_X86_SGX) += vsgx.o
@@ -105,6 +105,7 @@ CFLAGS_REMOVE_vclock_gettime.o = -pg
CFLAGS_REMOVE_vdso32/vclock_gettime.o = -pg
CFLAGS_REMOVE_vgetcpu.o = -pg
CFLAGS_REMOVE_vsgx.o = -pg
+CFLAGS_REMOVE_vgetrandom.o = -pg
#
# X32 processes use x32 vDSO to access 64bit kernel data.
diff --git a/arch/x86/entry/vdso/vdso.lds.S b/arch/x86/entry/vdso/vdso.lds.S
index e8c60ae7a7c8..0bab5f4af6d1 100644
--- a/arch/x86/entry/vdso/vdso.lds.S
+++ b/arch/x86/entry/vdso/vdso.lds.S
@@ -30,6 +30,8 @@ VERSION {
#ifdef CONFIG_X86_SGX
__vdso_sgx_enter_enclave;
#endif
+ getrandom;
+ __vdso_getrandom;
local: *;
};
}
diff --git a/arch/x86/entry/vdso/vgetrandom.c b/arch/x86/entry/vdso/vgetrandom.c
new file mode 100644
index 000000000000..157a6f7dbc44
--- /dev/null
+++ b/arch/x86/entry/vdso/vgetrandom.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Jason A. Donenfeld <[email protected]>. All Rights Reserved.
+ */
+#include <linux/types.h>
+
+#include "../../../../lib/vdso/getrandom.c"
+
+ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags);
+
+ssize_t __vdso_getrandom(void *buffer, size_t len, unsigned int flags)
+{
+ return __cvdso_getrandom(buffer, len, flags);
+}
+
+ssize_t getrandom(void *, size_t, unsigned int)
+ __attribute__((weak, alias("__vdso_getrandom")));
diff --git a/arch/x86/include/asm/vdso/getrandom.h b/arch/x86/include/asm/vdso/getrandom.h
new file mode 100644
index 000000000000..14247ddc431a
--- /dev/null
+++ b/arch/x86/include/asm/vdso/getrandom.h
@@ -0,0 +1,42 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2022 Jason A. Donenfeld <[email protected]>. All Rights Reserved.
+ */
+#ifndef __ASM_VDSO_GETRANDOM_H
+#define __ASM_VDSO_GETRANDOM_H
+
+#ifndef __ASSEMBLY__
+
+#include <asm/unistd.h>
+#include <asm/vvar.h>
+
+/**
+ * getrandom_syscall - Invoke the getrandom() syscall.
+ * @buffer: Input/Output buffer.
+ * @len: Size of @buffer in bytes.
+ * @flags: Zero or more GRND_* flags.
+ * Returns the number of random bytes written to @buffer, or a negative value indicating an error.
+ */
+static __always_inline ssize_t getrandom_syscall(void *buffer, size_t len, unsigned int flags)
+{
+ long ret;
+
+ asm ("syscall" : "=a" (ret) :
+ "0" (__NR_getrandom), "D" (buffer), "S" (len), "d" (flags) :
+ "rcx", "r11", "memory");
+
+ return ret;
+}
+
+#define __vdso_rng_data (VVAR(_vdso_rng_data))
+
+static __always_inline const struct vdso_rng_data *__arch_get_vdso_rng_data(void)
+{
+ if (__vdso_data->clock_mode == VDSO_CLOCKMODE_TIMENS)
+ return (void *)&__vdso_rng_data + ((void *)&__timens_vdso_data - (void *)&__vdso_data);
+ return &__vdso_rng_data;
+}
+
+#endif /* !__ASSEMBLY__ */
+
+#endif /* __ASM_VDSO_GETRANDOM_H */
diff --git a/arch/x86/include/asm/vdso/vsyscall.h b/arch/x86/include/asm/vdso/vsyscall.h
index be199a9b2676..71c56586a22f 100644
--- a/arch/x86/include/asm/vdso/vsyscall.h
+++ b/arch/x86/include/asm/vdso/vsyscall.h
@@ -11,6 +11,8 @@
#include <asm/vvar.h>
DEFINE_VVAR(struct vdso_data, _vdso_data);
+DEFINE_VVAR_SINGLE(struct vdso_rng_data, _vdso_rng_data);
+
/*
* Update the vDSO data page to keep in sync with kernel timekeeping.
*/
diff --git a/arch/x86/include/asm/vvar.h b/arch/x86/include/asm/vvar.h
index 183e98e49ab9..9d9af37f7cab 100644
--- a/arch/x86/include/asm/vvar.h
+++ b/arch/x86/include/asm/vvar.h
@@ -26,6 +26,8 @@
*/
#define DECLARE_VVAR(offset, type, name) \
EMIT_VVAR(name, offset)
+#define DECLARE_VVAR_SINGLE(offset, type, name) \
+ EMIT_VVAR(name, offset)
#else
@@ -37,6 +39,10 @@ extern char __vvar_page;
extern type timens_ ## name[CS_BASES] \
__attribute__((visibility("hidden"))); \
+#define DECLARE_VVAR_SINGLE(offset, type, name) \
+ extern type vvar_ ## name \
+ __attribute__((visibility("hidden"))); \
+
#define VVAR(name) (vvar_ ## name)
#define TIMENS(name) (timens_ ## name)
@@ -44,12 +50,22 @@ extern char __vvar_page;
type name[CS_BASES] \
__attribute__((section(".vvar_" #name), aligned(16))) __visible
+#define DEFINE_VVAR_SINGLE(type, name) \
+ type name \
+ __attribute__((section(".vvar_" #name), aligned(16))) __visible
+
#endif
/* DECLARE_VVAR(offset, type, name) */
DECLARE_VVAR(128, struct vdso_data, _vdso_data)
+#if !defined(_SINGLE_DATA)
+#define _SINGLE_DATA
+DECLARE_VVAR_SINGLE(640, struct vdso_rng_data, _vdso_rng_data)
+#endif
+
#undef DECLARE_VVAR
+#undef DECLARE_VVAR_SINGLE
#endif
--
2.37.2
Link: https://lore.kernel.org/all/[email protected]/
Signed-off-by: Yann Droneaud <[email protected]>
---
tools/testing/crypto/getrandom/Makefile | 4 +
.../testing/crypto/getrandom/test-getrandom.c | 307 ++++++++++++++++++
2 files changed, 311 insertions(+)
create mode 100644 tools/testing/crypto/getrandom/Makefile
create mode 100644 tools/testing/crypto/getrandom/test-getrandom.c
diff --git a/tools/testing/crypto/getrandom/Makefile b/tools/testing/crypto/getrandom/Makefile
new file mode 100644
index 000000000000..1370b6f1ae94
--- /dev/null
+++ b/tools/testing/crypto/getrandom/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0+
+
+test-getrandom: test-getrandom.c
+ $(CC) $(CPPFLAGS) $(CFLAGS) -I ../../../../usr/include/ -O2 -Wall -Wextra -o $@ $^ -ldl
diff --git a/tools/testing/crypto/getrandom/test-getrandom.c b/tools/testing/crypto/getrandom/test-getrandom.c
new file mode 100644
index 000000000000..311eef503f50
--- /dev/null
+++ b/tools/testing/crypto/getrandom/test-getrandom.c
@@ -0,0 +1,307 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2022 Yann Droneaud. All Rights Reserved.
+ */
+
+#include <dlfcn.h>
+#include <errno.h>
+#include <inttypes.h>
+#include <stdarg.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <sys/syscall.h>
+#include <time.h>
+#include <unistd.h>
+
+#include <linux/random.h>
+
+static size_t pagesz;
+static size_t discarded;
+
+typedef ssize_t(*getrandom_fn) (void *, size_t, int);
+
+static bool grnd_timestamp;
+static getrandom_fn getrandom_vDSO;
+
+static ssize_t getrandom_syscall(void *buffer, size_t size, int flags)
+{
+ return syscall(SYS_getrandom, buffer, size, flags);
+}
+
+static ssize_t timestamp(getrandom_fn _getrandom, uint64_t *grnd_ts,
+ size_t size)
+{
+ ssize_t ret;
+
+ ret = _getrandom(grnd_ts, size, GRND_TIMESTAMP);
+ if (ret < 0) {
+ fprintf(stderr,
+ "getrandom(,,GRND_TIMESTAMP) failed: %ld (%s)\n", -ret,
+ strerror((int)-ret));
+ return -1;
+ }
+
+ return ret;
+}
+
+static void fetch(getrandom_fn _getrandom, void *buffer, size_t size)
+{
+ ssize_t ret;
+
+ ret = _getrandom(buffer, size, 0);
+ if (ret < 0) {
+ fprintf(stderr, "getrandom(,,0) failed: %ld (%s)\n", -ret,
+ strerror((int)-ret));
+ exit(EXIT_FAILURE);
+ }
+}
+
+struct rng {
+ uint64_t grnd_ts;
+ size_t availsz; /* available bytes in buffer */
+ size_t buffersz; /* buffer size */
+ uint8_t buffer[];
+};
+
+static struct rng *rng;
+
+static void init_rng(void)
+{
+ int r;
+ ssize_t s;
+ void *p;
+
+ r = getpagesize();
+ if (r == -1) {
+ fprintf(stderr, "getpagesize() failed: %d\n", errno);
+ exit(EXIT_FAILURE);
+ }
+
+ pagesz = (size_t)r;
+
+ p = mmap(NULL, pagesz, PROT_READ | PROT_WRITE,
+ MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
+ if (p == MAP_FAILED) {
+ fprintf(stderr, "mmap() failed: %d\n", errno);
+ exit(EXIT_FAILURE);
+ }
+
+ r = madvise(p, pagesz, MADV_DONTDUMP | MADV_WIPEONFORK);
+ if (r == -1) {
+ fprintf(stderr, "madvise() failed: %d\n", errno);
+ exit(EXIT_FAILURE);
+ }
+
+ r = mlock(p, pagesz);
+ if (r == -1)
+ fprintf(stderr, "mlock() failed: %d\n", errno);
+
+ rng = p;
+
+ s = timestamp(getrandom_syscall, &rng->grnd_ts, sizeof(rng->grnd_ts));
+ if (s == -1)
+ return;
+
+ printf("getrandom() support GRND_TIMESTAMP\n");
+
+ grnd_timestamp = true;
+}
+
+static void init_vdso(void)
+{
+ void *h;
+ void *p;
+
+ h = dlopen("linux-vdso.so.1", RTLD_LAZY | RTLD_LOCAL | RTLD_NOLOAD);
+ if (!h) {
+ fprintf(stderr, "failed to open vDSO: %s\n", dlerror());
+ return;
+ }
+
+ p = dlsym(h, "__vdso_getrandom");
+ if (!p) {
+ fprintf(stderr, "getrandom() not found in vDSO: %s\n",
+ dlerror());
+ return;
+ }
+
+ printf("found getrandom() in vDSO at %p\n", p);
+
+ getrandom_vDSO = p;
+}
+
+/*
+ * 1) check timestamp isn't expired
+ * 2) if expired or there's not enough data in buffer
+ * a) if expired, reset buffer size,
+ * b) fetch new random stream
+ * c) check timestamp
+ * d) if expired, reset buffer size, go to b)
+ *
+ */
+static void ensure(getrandom_fn _getrandom, size_t request)
+{
+ ssize_t r;
+
+ r = timestamp(_getrandom, &rng->grnd_ts, sizeof(rng->grnd_ts));
+ switch (r) {
+ case 0: /* timestamp didn't change */
+ /* enough available random bytes ? */
+ if (rng->availsz >= request)
+ return;
+
+ /* increase buffer size when drained */
+ if (rng->buffersz < pagesz - sizeof(*rng))
+ rng->buffersz *= 2;
+
+ /* no less than 32 */
+ if (rng->buffersz < 32)
+ rng->buffersz = 32;
+
+ /* no more than a full page minus the rng structure */
+ if (rng->buffersz > pagesz - sizeof(*rng))
+ rng->buffersz = pagesz - sizeof(*rng);
+
+ break;
+
+ case sizeof(rng->grnd_ts): /* timestamp did change, random bytes must be discarded */
+ rng->buffersz = 32; /* reset size */
+ break;
+
+ default:
+ fprintf(stderr, "unexpected timestamp size %zd\n", r);
+ exit(EXIT_FAILURE);
+ }
+
+ /* keep fetching if timestamp is updated */
+ for (;;) {
+ if (rng->availsz)
+ discarded += rng->availsz;
+
+ fetch(_getrandom, rng->buffer, rng->buffersz);
+ rng->availsz = rng->buffersz;
+
+ r = timestamp(_getrandom, &rng->grnd_ts, sizeof(rng->grnd_ts));
+
+ switch (r) {
+ case 0: /* timestamp didn't change between previous check and last fetch */
+ return;
+
+ case sizeof(rng->grnd_ts): /* timestamp did change, random bytes just fetched must be discarded */
+ rng->buffersz = 32; /* reset size */
+ continue; /* retry again */
+
+ default:
+ fprintf(stderr, "unexpected timestamp size %zd\n", r);
+ exit(EXIT_FAILURE);
+ }
+ }
+}
+
+/* arc4random() */
+static void get_direct(getrandom_fn _getrandom)
+{
+ uint32_t v;
+ fetch(_getrandom, &v, sizeof(v));
+}
+
+static void get_pooled(getrandom_fn _getrandom)
+{
+ ensure(_getrandom, sizeof(uint32_t));
+ rng->availsz -= sizeof(uint32_t);
+}
+
+static inline struct timespec timespec_sub(const struct timespec *a,
+ const struct timespec *b)
+{
+ struct timespec res;
+
+ res.tv_sec = a->tv_sec - b->tv_sec;
+ res.tv_nsec = a->tv_nsec - b->tv_nsec;
+ if (res.tv_nsec < 0) {
+ res.tv_sec--;
+ res.tv_nsec += 1000000000L;
+ }
+
+ return res;
+}
+
+#define SAMPLES 13
+#define VALUES (16 * 1024 * 1024)
+
+static void test_direct(getrandom_fn _getrandom, const char *method)
+{
+ struct timespec start, end, diff;
+
+ for (int i = 0; i < SAMPLES; i++) {
+ clock_gettime(CLOCK_MONOTONIC, &start);
+
+ for (uint32_t j = 0; j < VALUES; j++)
+ get_direct(_getrandom);
+
+ clock_gettime(CLOCK_MONOTONIC, &end);
+
+ diff = timespec_sub(&end, &start);
+
+ printf("== direct %s getrandom(), %u u32, %lu.%09lu s, %.3f M u32/s, %.3f ns/u32\n",
+ method, VALUES, diff.tv_sec, diff.tv_nsec,
+ VALUES / (1000000 *
+ (diff.tv_sec +
+ (double)diff.tv_nsec / 1000000000UL)),
+ (double)(diff.tv_sec * 1000000000UL +
+ diff.tv_nsec) / VALUES);
+ }
+}
+
+static void test_pooled(getrandom_fn _getrandom, const char *method)
+{
+ struct timespec start, end, diff;
+
+ for (int i = 0; i < SAMPLES; i++) {
+ discarded = 0;
+
+ clock_gettime(CLOCK_MONOTONIC, &start);
+
+ for (uint32_t j = 0; j < VALUES; j++)
+ get_pooled(_getrandom);
+
+ clock_gettime(CLOCK_MONOTONIC, &end);
+
+ diff = timespec_sub(&end, &start);
+
+ printf("== pooled %s getrandom(), %u u32, %lu.%09lu s, %.3f M u32/s, %.3f ns/u32, (%zu bytes discarded)\n",
+ method, VALUES, diff.tv_sec, diff.tv_nsec,
+ VALUES / (1000000 *
+ (diff.tv_sec +
+ (double)diff.tv_nsec / 1000000000UL)),
+ (double)(diff.tv_sec * 1000000000UL +
+ diff.tv_nsec) / VALUES,
+ discarded);
+ }
+}
+
+int main(void)
+{
+ printf("getrandom(,,GRND_TIMESTAMP) test\n");
+
+ init_rng();
+ init_vdso();
+
+ while (1) {
+ test_direct(getrandom_syscall, "syscall");
+
+ if (getrandom_vDSO)
+ test_direct(getrandom_vDSO, "vDSO");
+
+ if (grnd_timestamp)
+ test_pooled(getrandom_syscall, "syscall");
+
+ if (getrandom_vDSO && grnd_timestamp)
+ test_pooled(getrandom_vDSO, "vDSO");
+ }
+
+ return 0;
+}
--
2.37.2
Sorry Yann, but I'm not interested in this approach, and I don't think
reviewing the details of it are a good allocation of time. I don't
want to lock the kernel into having specific reseeding semantics that
are a contract with userspace, which is what this approach does.
Please just let me iterate on my original patchset for a little bit,
without adding more junk to the already overly large conversation.
Hi
12 janvier 2023 à 18:07 "Jason A. Donenfeld" <[email protected]> a écrit:
> Sorry Yann, but I'm not interested in this approach, and I don't think
> reviewing the details of it are a good allocation of time. I don't
> want to lock the kernel into having specific reseeding semantics that
> are a contract with userspace, which is what this approach does.
This patch adds a mean for the kernel to tell userspace: between the
last time you call us with getrandom(timestamp,, GRND_TIMESTAMP),
something happened that trigger an update to the opaque cookie given
to getrandom(timestamp, GRND_TIMESTAMP). When such update happen,
userspace is advised to discard buffered random data and retry.
The meaning of the timestamp cookie is up to the kernel, and can be
changed anytime. Userspace is not expected to read the content of this
blob. Userspace only acts on the length returned by getrandom(,, GRND_TIMESTAMP):
-1 : not supported
0 : cookie not updated, no need to discard buffered data
>0 : cookie updated, userspace should discard buffered data
For the cookie, I've used a single u64, but two u64 could be a better start,
providing room for implementing improved behavior in future kernel versions.
> Please just let me iterate on my original patchset for a little bit,
> without adding more junk to the already overly large conversation.
I like the simplicity of my so called "junk". It's streamlined, doesn't
require a new syscall, doesn't require a new copy of ChaCha20 code.
I'm sorry it doesn't fit your expectations.
Regards.
--
Yann Droneaud
OPTEYA
[...]
>GRND_TIMESTAMP allows userspace to ask the kernel if previous
>"timestamp" has changed as the result of an event that
>triggered kernel CSPRNG reseed, and to update the "timestamp".
>In case the "timestamp" hasn't changed, userspace CSPRNG can
>consume a slice of its buffered random stream.
>If it has changed, remaining userspace buffered random values
>should be discarded. Userspace should call getrandom() to fill
>and/or generate its buffer with updated seed.
>It's advised to test again the "timestamp" to deal with the
>race condition, where the kernel reseed just after the call
>to getrandom() to get entropy.
This second check is the 'safe thing' to do, but if you're that
worried about race conditions then this api is useless. You can't
ignore the inherent TOCTOU problem with the GRND_TIMESTAMP calls.
That said, the race condition is so tiny that the added overhead
of a third syscall (without vDSO support) starts to negate the
value in buffering the random numbers (not to mention adding
significant latency to every nth arc4random() call, for example).
IMO, for callers that cannot accept the risk, the getrandom(,,0)
option is the perfect alternative.
[...]
>+static ssize_t get_random_timestamp(char __user *ubuf, size_t len, unsigned int flags)
>+{
>+ u64 ts;
>+
>+ /* other combination not supported */
>+ if (WARN(flags != GRND_TIMESTAMP, "GRND_TIMESTAMP cannot be used with other flags"))
>+ return -EINVAL;
If userspace messes up the flags then it's the problem of the
caller. Why clutter the logs in that case?
At the very least this should be WARN_ONCE() to avoid log spam.
>+ /* shorter structure not supported */
>+ if (len < sizeof(ts))
>+ return -EINVAL;
This should be sizeof(u64) to match the vDSO patch and to avoid
having to change this condition if ts becomes larger.
>+
>+ if (copy_from_user(&ts, ubuf, sizeof(ts)))
>+ return -EFAULT;
>+
>+ /* longer structure supported, only if 0-padded,
>+ timestamp might be extended in the future with more fields */
>+ if (len > sizeof(ts)) {
>+ char __user *p = ubuf + sizeof(ts);
>+ size_t l = len - sizeof(ts);
>+
>+ while (l) {
>+ char b;
>+
>+ if (get_user(b, p++))
>+ return -EFAULT;
>+
>+ if (b)
>+ return -EINVAL;
>+ }
>+ }
>+
>+ if (!get_random_timestamp_update(&ts, READ_ONCE(base_crng.generation)))
>+ return 0;
>+
>+ if (copy_to_user(ubuf, &ts, sizeof(ts)))
>+ return -EFAULT;
>+
>+ return sizeof(ts);
>+}
>+
> SYSCALL_DEFINE3(getrandom, char __user *, ubuf, size_t, len, unsigned int, flags)
> {
> struct iov_iter iter;
> struct iovec iov;
> int ret;
>
>- if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE))
>+ if (flags & ~(GRND_NONBLOCK | GRND_RANDOM | GRND_INSECURE | GRND_TIMESTAMP))
> return -EINVAL;
>
>+ if (unlikely(flags & GRND_TIMESTAMP))
>+ return get_random_timestamp(ubuf, len, flags);
>+
I'd remove the unlikely(). I don't like to assume usage patterns.
> /*
> * Requesting insecure and blocking randomness at the same time makes
> * no sense.
>diff --git a/include/linux/random.h b/include/linux/random.h
>index b0a940af4fff..bc219b5a96a5 100644
>--- a/include/linux/random.h
>+++ b/include/linux/random.h
>@@ -161,4 +161,35 @@ int random_online_cpu(unsigned int cpu);
> extern const struct file_operations random_fops, urandom_fops;
> #endif
>
>+/*
>+ * get_random_timestamp_update()
>+ *
>+ * @generation: current CRNG generation (from base_crng.generation
>+ * or _vdso_rng_data.generation)
>+ *
>+ * Return: timestamp size if previous timestamp is expired and is updated,
>+ * 0 if not expired (and not updated)
>+ */
>+static inline bool get_random_timestamp_update(u64 *user_ts,
>+ u64 generation)
>+{
>+ u64 ts;
>+
>+ /* base_crng.generation is never ULONG_MAX,
>+ * OTOH userspace will initialize its timestamp
>+ * to 0, so inverting base_crng.generation ensure
>+ * first call to getrandom(,,GRND_TIMESTAMP) will
>+ * update
>+ */
Rather than assuming that the timestamp will start zero-initilized,
expect it to be uninitilized. Either way the code works.
>+ ts = ~generation;
>+
>+ /* not expired ? no refresh suggested */
>+ if (*user_ts == ts)
>+ return false;
>+
>+ *user_ts = ts;
>+
>+ return true;
>+}
>+
Not that it matters much, but you could make generation a u64* that
gets dereferenced by get_random_timestamp_update(). It's cleaner for
the caller, barely changes this function, and will get inlined anyway.
I might just be imposing my personal style in this case though.
After a cursory skimming of the rest of the series I think that this
is a worthwhile direction to pursue. Jason's series is growing bulky
and this provides the needed slimming while solving the root problem.
The only thing I see immediately is the TOCTOU problem and the extra
steps needed to guarantee forward secrecy.
I should mention that I'm not a security or rng expert at all.
Cheers,
Peter Lafreniere
<[email protected]>
On 1/12/23 11:55, Yann Droneaud wrote:
> Hi
>
> 12 janvier 2023 à 18:07 "Jason A. Donenfeld" <[email protected]> a écrit:
>
>> Sorry Yann, but I'm not interested in this approach, and I don't think
>> reviewing the details of it are a good allocation of time. I don't
>> want to lock the kernel into having specific reseeding semantics that
>> are a contract with userspace, which is what this approach does.
>
> This patch adds a mean for the kernel to tell userspace: between the
> last time you call us with getrandom(timestamp,, GRND_TIMESTAMP),
> something happened that trigger an update to the opaque cookie given
> to getrandom(timestamp, GRND_TIMESTAMP). When such update happen,
> userspace is advised to discard buffered random data and retry.
>
> The meaning of the timestamp cookie is up to the kernel, and can be
> changed anytime. Userspace is not expected to read the content of this
> blob. Userspace only acts on the length returned by getrandom(,, GRND_TIMESTAMP):
> -1 : not supported
> 0 : cookie not updated, no need to discard buffered data
> >0 : cookie updated, userspace should discard buffered data
>
> For the cookie, I've used a single u64, but two u64 could be a better start,
> providing room for implementing improved behavior in future kernel versions.
>
>> Please just let me iterate on my original patchset for a little bit,
>> without adding more junk to the already overly large conversation.
>
> I like the simplicity of my so called "junk". It's streamlined, doesn't
> require a new syscall, doesn't require a new copy of ChaCha20 code.
>
> I'm sorry it doesn't fit your expectations.
>
Why would anything more than a 64-bit counter be ever necessary? It only
needs to be incremented.
Let user space manage keeping track of the cookie matching its own
buffers. You do NOT want this to be stateful, because that's just
begging for multiple libraries to step on each other.
Export the cookie from the vdso and volià, a very cheap check around any
user space randomness buffer will work:
static clone_cookie_t last_cookie;
clone_cookie_t this_cookie;
this_cookie = get_clone_cookie();
do {
while (this_cookie != last_cookie) {
last_cookie = this_cookie;
reinit_randomness();
this_cookie = get_clone_cookie();
}
extract_randomness_from_buffer();
this_cookie = get_clone_cookie();
} while (this_cookie != last_cookie);
last_cookie = this_cookie;
-hpa
> On Jan 13, 2023, at 7:16 PM, H. Peter Anvin <[email protected]> wrote:
>
> On 1/12/23 11:55, Yann Droneaud wrote:
>> Hi
>> 12 janvier 2023 à 18:07 "Jason A. Donenfeld" <[email protected]> a écrit:
>>
>>> Sorry Yann, but I'm not interested in this approach, and I don't think
>>> reviewing the details of it are a good allocation of time. I don't
>>> want to lock the kernel into having specific reseeding semantics that
>>> are a contract with userspace, which is what this approach does.
>> This patch adds a mean for the kernel to tell userspace: between the
>> last time you call us with getrandom(timestamp,, GRND_TIMESTAMP),
>> something happened that trigger an update to the opaque cookie given
>> to getrandom(timestamp, GRND_TIMESTAMP). When such update happen,
>> userspace is advised to discard buffered random data and retry.
>> The meaning of the timestamp cookie is up to the kernel, and can be
>> changed anytime. Userspace is not expected to read the content of this
>> blob. Userspace only acts on the length returned by getrandom(,, GRND_TIMESTAMP):
>> -1 : not supported
>> 0 : cookie not updated, no need to discard buffered data
>> >0 : cookie updated, userspace should discard buffered data
>> For the cookie, I've used a single u64, but two u64 could be a better start,
>> providing room for implementing improved behavior in future kernel versions.
>>> Please just let me iterate on my original patchset for a little bit,
>>> without adding more junk to the already overly large conversation.
>> I like the simplicity of my so called "junk". It's streamlined, doesn't
>> require a new syscall, doesn't require a new copy of ChaCha20 code.
>> I'm sorry it doesn't fit your expectations.
>
> Why would anything more than a 64-bit counter be ever necessary? It only needs to be incremented.
This is completely broken with CRIU or, for that matter, with VM forking.
>
> Let user space manage keeping track of the cookie matching its own buffers. You do NOT want this to be stateful, because that's just begging for multiple libraries to step on each other.
>
> Export the cookie from the vdso and volià, a very cheap check around any user space randomness buffer will work:
>
> static clone_cookie_t last_cookie;
> clone_cookie_t this_cookie;
>
> this_cookie = get_clone_cookie();
> do {
> while (this_cookie != last_cookie) {
> last_cookie = this_cookie;
> reinit_randomness();
> this_cookie = get_clone_cookie();
> }
>
> extract_randomness_from_buffer();
> this_cookie = get_clone_cookie();
> } while (this_cookie != last_cookie);
>
> last_cookie = this_cookie;
>
> -hpa
On January 16, 2023 11:49:42 AM PST, Andy Lutomirski <[email protected]> wrote:
>
>
>> On Jan 13, 2023, at 7:16 PM, H. Peter Anvin <[email protected]> wrote:
>>
>> On 1/12/23 11:55, Yann Droneaud wrote:
>>> Hi
>>> 12 janvier 2023 à 18:07 "Jason A. Donenfeld" <[email protected]> a écrit:
>>>
>>>> Sorry Yann, but I'm not interested in this approach, and I don't think
>>>> reviewing the details of it are a good allocation of time. I don't
>>>> want to lock the kernel into having specific reseeding semantics that
>>>> are a contract with userspace, which is what this approach does.
>>> This patch adds a mean for the kernel to tell userspace: between the
>>> last time you call us with getrandom(timestamp,, GRND_TIMESTAMP),
>>> something happened that trigger an update to the opaque cookie given
>>> to getrandom(timestamp, GRND_TIMESTAMP). When such update happen,
>>> userspace is advised to discard buffered random data and retry.
>>> The meaning of the timestamp cookie is up to the kernel, and can be
>>> changed anytime. Userspace is not expected to read the content of this
>>> blob. Userspace only acts on the length returned by getrandom(,, GRND_TIMESTAMP):
>>> -1 : not supported
>>> 0 : cookie not updated, no need to discard buffered data
>>> >0 : cookie updated, userspace should discard buffered data
>>> For the cookie, I've used a single u64, but two u64 could be a better start,
>>> providing room for implementing improved behavior in future kernel versions.
>>>> Please just let me iterate on my original patchset for a little bit,
>>>> without adding more junk to the already overly large conversation.
>>> I like the simplicity of my so called "junk". It's streamlined, doesn't
>>> require a new syscall, doesn't require a new copy of ChaCha20 code.
>>> I'm sorry it doesn't fit your expectations.
>>
>> Why would anything more than a 64-bit counter be ever necessary? It only needs to be incremented.
>
>This is completely broken with CRIU or, for that matter, with VM forking.
>
>>
>> Let user space manage keeping track of the cookie matching its own buffers. You do NOT want this to be stateful, because that's just begging for multiple libraries to step on each other.
>>
>> Export the cookie from the vdso and volià, a very cheap check around any user space randomness buffer will work:
>>
>> static clone_cookie_t last_cookie;
>> clone_cookie_t this_cookie;
>>
>> this_cookie = get_clone_cookie();
>> do {
>> while (this_cookie != last_cookie) {
>> last_cookie = this_cookie;
>> reinit_randomness();
>> this_cookie = get_clone_cookie();
>> }
>>
>> extract_randomness_from_buffer();
>> this_cookie = get_clone_cookie();
>> } while (this_cookie != last_cookie);
>>
>> last_cookie = this_cookie;
>>
>> -hpa
>
For those you would randomize the counter.
Hi,
16 janvier 2023 à 20:50 "Andy Lutomirski" <[email protected]> a écrit:
> > On Jan 13, 2023, at 7:16 PM, H. Peter Anvin <[email protected]> wrote:
> > On 1/12/23 11:55, Yann Droneaud wrote:
> > > 12 janvier 2023 à 18:07 "Jason A. Donenfeld" <[email protected]> a écrit:
> > >
> >
> > Sorry Yann, but I'm not interested in this approach, and I don't think
> > reviewing the details of it are a good allocation of time. I don't
> > want to lock the kernel into having specific reseeding semantics that
> > are a contract with userspace, which is what this approach does.
> >
> > >
> > > This patch adds a mean for the kernel to tell userspace: between the
> > > last time you call us with getrandom(timestamp,, GRND_TIMESTAMP),
> > > something happened that trigger an update to the opaque cookie given
> > > to getrandom(timestamp, GRND_TIMESTAMP). When such update happen,
> > > userspace is advised to discard buffered random data and retry.
> > > The meaning of the timestamp cookie is up to the kernel, and can be
> > > changed anytime. Userspace is not expected to read the content of this
> > > blob. Userspace only acts on the length returned by getrandom(,, GRND_TIMESTAMP):
> > > -1 : not supported
> > > 0 : cookie not updated, no need to discard buffered data
> > > >0 : cookie updated, userspace should discard buffered data
> > > For the cookie, I've used a single u64, but two u64 could be a better start,
> > > providing room for implementing improved behavior in future kernel versions.
> > >
> >
> > Please just let me iterate on my original patchset for a little bit,
> > without adding more junk to the already overly large conversation.
> >
> > >
> > > I like the simplicity of my so called "junk". It's streamlined, doesn't
> > > require a new syscall, doesn't require a new copy of ChaCha20 code.
> > > I'm sorry it doesn't fit your expectations.
> > >
> >
> >
> > Why would anything more than a 64-bit counter be ever necessary? It only needs to be incremented.
> >
>
> This is completely broken with CRIU or, for that matter, with VM forking.
>
Which raise the question of the support of CRIU with Jason's vDSO proposal.
AFAIK CRIU handle vDSO[1] by interposing symbols so that, on restore, the process
will call the interposed functions, which will resolve the new vDSO's functions.
vgetrandom_alloc() would have been called before the checkpoint, allocating one
opaque state of size x. After the restore, the vDSO's getrandom() would be given
this opaque state, expecting it having size y. As the content of the opaque state
should have been cleared per MADV_WIPEONFORK, there's nothing in the state that
could help vDSO's getrandom() to achieve backward compatibility.
I think backward compatibility can be achieved by adding an opaque state size
argument to vDSO's getrandom().
What to think Jason ?
[1] https://criu.org/Vdso
Regards.
--
Yann Droneaud
OPTEYA