Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: 2022-11-30 16:40:49
Also in:
linux-crypto, linux-patches, lkml
Subsystem:
abi/api, random number driver, the rest · Maintainers:
"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:
2) Convert vgetrandom_alloc() into a clone3-style syscall, as Christian
suggested earlier, which might allow for a bit more overloading
capability. That would be a struct that looks like:
struct vgetrandom_alloc_args {
__aligned_u64 flags;
__aligned_u64 states;
__aligned_u64 num;
__aligned_u64 size_of_each;
}
- If flags is VGRA_ALLOCATE, states and size_of_each must be zero on
input, while num is the hint, as is the case now. On output, states,
size_of_each, and num are filled in.
- If flags is VGRA_DEALLOCATE, states, size_of_each, and num must be as
they were originally, and then it deallocates.
I suppose (2) would alleviate your concerns entirely, without future
uncertainty over what it'd be like to add special cases to munmap(). And
it'd add a bit more future proofing to the syscall, depending on what we
do.
So maybe I'm warming up to that approach a bit.So I just did a little quick implementation to see what it'd feel like, and actually, it's quite simple, and might address a lot of concerns all at once. What do you think of the below? Documentation and such still needs work obviously, but the bones should be there.
diff --git a/drivers/char/random.c b/drivers/char/random.c
index 4341c6a91207..dae6095b937d 100644
--- a/drivers/char/random.c
+++ b/drivers/char/random.c@@ -189,44 +189,53 @@ int __cold execute_with_initialized_rng(struct notifier_block *nb) /** * sys_vgetrandom_alloc - Allocate opaque states for use with vDSO getrandom(). * - * @num: On input, a pointer to a suggested hint of how many states to - * allocate, and on output the number of states actually allocated. - * - * @size_per_each: The size of each state allocated, so that the caller can - * split up the returned allocation into individual states. - * - * @flags: Currently always zero. + * @uargs: A vgetrandom_alloc_args which may be updated on return. + * allocate, and on output the number of states actually allocated. + * @usize: The size of @uargs, which determines the version of the struct used. * * The getrandom() vDSO function in userspace requires an opaque state, which * this function allocates 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 provides the caller with the number of opaque states actually * allocated, the size of each one in bytes, and the address of the first - * state. + * state. Alternatively, if the VGRA_DEALLOCATE flag is specified, the provided + * states parameter is unmapped. * - * Returns the address of the first state in the allocation on success, or a - * negative error value on failure. + * Returns 0 on success and an error value otherwise. */ -SYSCALL_DEFINE3(vgetrandom_alloc, unsigned int __user *, num, - unsigned int __user *, size_per_each, unsigned int, flags) +SYSCALL_DEFINE2(vgetrandom_alloc, struct vgetrandom_alloc_args __user *, uargs, size_t, usize) { const size_t state_size = sizeof(struct vgetrandom_state); + const size_t max_states = (SIZE_MAX & PAGE_MASK) / state_size; + struct vgetrandom_alloc_args args; size_t alloc_size, num_states; unsigned long pages_addr; - unsigned int num_hint; int ret; - if (flags) + if (usize > PAGE_SIZE) + return -E2BIG; + if (usize < VGETRANDOM_ALLOC_ARGS_SIZE_VER0) return -EINVAL; + ret = copy_struct_from_user(&args, sizeof(args), uargs, usize); + if (ret) + return ret; - if (get_user(num_hint, num)) - return -EFAULT; + /* Currently only VGRA_DEALLOCATE is defined. */ + if (args.flags & ~VGRA_DEALLOCATE) + return -EINVAL; - num_states = clamp_t(size_t, num_hint, 1, (SIZE_MAX & PAGE_MASK) / state_size); - alloc_size = PAGE_ALIGN(num_states * state_size); + 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); + } - if (put_user(alloc_size / state_size, num) || put_user(state_size, size_per_each)) - return -EFAULT; + /* These don't make sense as input values if allocating, so reject them. */ + if (args.size_per_each || args.states) + return -EINVAL; + + num_states = clamp_t(size_t, args.num, 1, max_states); + alloc_size = PAGE_ALIGN(num_states * state_size); pages_addr = vm_mmap(NULL, 0, alloc_size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_LOCKED, 0);
@@ -237,7 +246,14 @@ SYSCALL_DEFINE3(vgetrandom_alloc, unsigned int __user *, num, if (ret < 0) goto err_unmap; - return pages_addr; + args.num = num_states; + args.size_per_each = state_size; + args.states = pages_addr; + + ret = -EFAULT; + if (copy_to_user(uargs, &args, sizeof(args))) + goto err_unmap; + return 0; err_unmap: vm_munmap(pages_addr, alloc_size);
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index 7741dc94f10c..de4338e26db0 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h@@ -72,6 +72,7 @@ struct open_how; struct mount_attr; struct landlock_ruleset_attr; enum landlock_rule_type; +struct vgetrandom_alloc_args; #include <linux/types.h> #include <linux/aio_abi.h>
@@ -1006,9 +1007,8 @@ asmlinkage long sys_seccomp(unsigned int op, unsigned int flags, void __user *uargs); asmlinkage long sys_getrandom(char __user *buf, size_t count, unsigned int flags); -asmlinkage long sys_vgetrandom_alloc(unsigned int __user *num, - unsigned int __user *size_per_each, - unsigned int flags); +asmlinkage long sys_vgetrandom_alloc(struct vgetrandom_alloc_args __user *uargs, + size_t size); asmlinkage long sys_memfd_create(const char __user *uname_ptr, unsigned int flags); asmlinkage long sys_bpf(int cmd, union bpf_attr *attr, unsigned int size); asmlinkage long sys_execveat(int dfd, const char __user *filename,
diff --git a/include/uapi/linux/random.h b/include/uapi/linux/random.h
index e744c23582eb..49911ea2c343 100644
--- a/include/uapi/linux/random.h
+++ b/include/uapi/linux/random.h@@ -55,4 +55,30 @@ struct rand_pool_info { #define GRND_RANDOM 0x0002 #define GRND_INSECURE 0x0004 +/* + * Flags for vgetrandom_alloc(2) + * + * VGRA_DEALLOCATE Deallocate supplied states. + */ +#define VGRA_DEALLOCATE 0x0001ULL + +/** + * struct vgetrandom_alloc_args - Arguments for the vgetrandom_alloc(2) syscall. + * + * @flags: Zero or more VGRA_* flags. + * @states: Zero on input if allocating, and filled in on successful + * return. An existing allocation, if deallocating. + * @num: A hint as to the desired number of states, if allocating. The + * number of existing states in @states, if deallocating + * @size_per_each: The size of each state in @states. + */ +struct vgetrandom_alloc_args { + __aligned_u64 flags; + __aligned_u64 states; + __aligned_u64 num; + __aligned_u64 size_per_each; +}; + +#define VGETRANDOM_ALLOC_ARGS_SIZE_VER0 32 /* sizeof first published struct */ + #endif /* _UAPI_LINUX_RANDOM_H */