Re: [PATCH v7 1/3] random: add vgetrandom_alloc() syscall
From: Thomas Gleixner <hidden>
Date: 2022-11-25 20:45:39
Also in:
linux-crypto, linux-patches, lkml
On Thu, Nov 24 2022 at 17:55, Jason A. Donenfeld wrote:
--- MAINTAINERS | 1 + arch/x86/Kconfig | 1 + arch/x86/entry/syscalls/syscall_64.tbl | 1 + arch/x86/include/asm/unistd.h | 1 + drivers/char/random.c | 59 +++++++++++++++++++++++++ include/uapi/asm-generic/unistd.h | 7 ++- kernel/sys_ni.c | 3 ++ lib/vdso/getrandom.h | 23 ++++++++++ scripts/checksyscalls.sh | 4 ++ tools/include/uapi/asm-generic/unistd.h | 7 ++- 10 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 lib/vdso/getrandom.h
I think I asked for this before: Please split these things properly up. Provide the syscall and then wire it up.
quoted hunk ↗ jump to hunk
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig index 67745ceab0db..331e21ba961a 100644 --- a/arch/x86/Kconfig +++ b/arch/x86/Kconfig@@ -59,6 +59,7 @@ config X86 # select ACPI_LEGACY_TABLES_LOOKUP if ACPI select ACPI_SYSTEM_POWER_STATES_SUPPORT if ACPI + select ADVISE_SYSCALLS if X86_64
Why is this x86_64 specific?
quoted hunk ↗ jump to hunk
--- a/arch/x86/include/asm/unistd.h +++ b/arch/x86/include/asm/unistd.h@@ -27,6 +27,7 @@ # define __ARCH_WANT_COMPAT_SYS_PWRITEV64 # define __ARCH_WANT_COMPAT_SYS_PREADV64V2 # define __ARCH_WANT_COMPAT_SYS_PWRITEV64V2 +# define __ARCH_WANT_VGETRANDOM_ALLOC
So instead of this define, why can't you do:
config VGETRADOM_ALLOC
bool
select ADVISE_SYSCALLS
and then have
config GENERIC_VDSO_RANDOM_WHATEVER
bool
select VGETRANDOM_ALLOC
This gives a clear Kconfig dependency instead of the random
ADVISE_SYSCALLS select.
quoted hunk ↗ jump to hunk
--- a/drivers/char/random.c +++ b/drivers/char/random.c
+#include "../../lib/vdso/getrandom.h"
Seriously? include/vdso/ exists for a reason.
+#ifdef __ARCH_WANT_VGETRANDOM_ALLOC +/* + * The vgetrandom() function in userspace requires an opaque state, which this + * function provides to userspace, by mapping a certain number of special pages + * into the calling process. It takes a hint as to the number of opaque states + * desired, and returns the number of opaque states actually allocated, the + * size of each one in bytes, and the address of the first state.
As this is a syscall which can be invoked outside of the VDSO, can you please provide proper kernel-doc which explains the arguments, the functionality and the return value?
+ */
+SYSCALL_DEFINE3(vgetrandom_alloc, unsigned int __user *, num,
+ unsigned int __user *, size_per_each, unsigned int, flags)
+{
+ size_t alloc_size, num_states;
+ unsigned long pages_addr;
+ unsigned int num_hint;
+ int ret;
+
+ if (flags)
+ return -EINVAL;
+
+ if (get_user(num_hint, num))
+ return -EFAULT;
+
+ num_states = clamp_t(size_t, num_hint, 1, (SIZE_MAX & PAGE_MASK) / sizeof(struct vgetrandom_state));
+ alloc_size = PAGE_ALIGN(num_states * sizeof(struct vgetrandom_state));
+
+ if (put_user(alloc_size / sizeof(struct vgetrandom_state), num) ||
+ put_user(sizeof(struct vgetrandom_state), size_per_each))
+ return -EFAULT;
That's a total of four sizeof(struct vgetrandom_state) usage sites.
size_t state_size = sizeof(struct vgetrandom_state);
perhaps?
quoted hunk ↗ jump to hunk
diff --git a/lib/vdso/getrandom.h b/lib/vdso/getrandom.h new file mode 100644 index 000000000000..c7f727db2aaa --- /dev/null +++ b/lib/vdso/getrandom.h
Wrong place. See above.
quoted hunk ↗ jump to hunk
@@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2022 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved. + */ + +#ifndef _VDSO_LIB_GETRANDOM_H +#define _VDSO_LIB_GETRANDOM_H + +#include <crypto/chacha.h> + +struct vgetrandom_state { + union { + struct { + u8 batch[CHACHA_BLOCK_SIZE * 3 / 2]; + u32 key[CHACHA_KEY_SIZE / sizeof(u32)]; + }; + u8 batch_key[CHACHA_BLOCK_SIZE * 2]; + }; + unsigned long generation; + u8 pos; +};
Thanks,
tglx