Message-ID: <Y4eG9cUE28s0YpgO@zx2c4.com>
Date:   Wed, 30 Nov 2022 17:38:13 +0100
From:   "Jason A. Donenfeld" <Jason@...c4.com>
To:     Florian Weimer <fweimer@...hat.com>
Cc:     linux-kernel@...r.kernel.org, patches@...ts.linux.dev,
        tglx@...utronix.de, linux-crypto@...r.kernel.org,
        linux-api@...r.kernel.org, x86@...nel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Adhemerval Zanella Netto <adhemerval.zanella@...aro.org>,
        Carlos O'Donell <carlos@...hat.com>,
        Arnd Bergmann <arnd@...db.de>,
        Christian Brauner <brauner@...nel.org>
Subject: Re: [PATCH v10 1/4] random: add vgetrandom_alloc() syscall

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 */

Powered by blists - more mailing lists