[PATCH v1 0/4] lib: introduce copy_struct_from_user() helper

[PATCH v1 0/4] lib: introduce copy_struct_from_user() helper

[Aleksa Sarai]

This series was split off from the openat2(2) syscall discussion[1].
However, the copy_struct_to_user() helper has been dropped, because
after some discussion it appears that there is no really obvious
semantics for how copy_struct_to_user() should work on mixed-vintages
(for instance, whether [2] is the correct semantics for all syscalls).

A common pattern for syscall extensions is increasing the size of a
struct passed from userspace, such that the zero-value of the new fields
result in the old kernel behaviour (allowing for a mix of userspace and
kernel vintages to operate on one another in most cases).

Previously there was no common lib/ function that implemented
the necessary extension-checking semantics (and different syscalls
implemented them slightly differently or incompletely[3]). This series
implements the helper and ports several syscalls to use it.

[1]: https://lore.kernel.org/lkml/20190904201933.10736-1-cyphar@cyphar.com/

[2]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
     robustify sched_read_attr() ABI logic and code")

[3]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
     similar checks to copy_struct_from_user() while rt_sigprocmask(2)
     always rejects differently-sized struct arguments.

Aleksa Sarai (4):
  lib: introduce copy_struct_from_user() helper
  clone3: switch to copy_struct_from_user()
  sched_setattr: switch to copy_struct_from_user()
  perf_event_open: switch to copy_struct_from_user()

 include/linux/uaccess.h    |  4 +++
 include/uapi/linux/sched.h |  2 ++
 kernel/events/core.c       | 47 +++++-------------------
 kernel/fork.c              | 34 ++++--------------
 kernel/sched/core.c        | 43 ++++------------------
 lib/Makefile               |  2 +-
 lib/strnlen_user.c         | 52 +++++++++++++++++++++++++++
 lib/struct_user.c          | 73 ++++++++++++++++++++++++++++++++++++++
 8 files changed, 155 insertions(+), 102 deletions(-)
 create mode 100644 lib/struct_user.c

-- 
2.23.0

[PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Aleksa Sarai]

A common pattern for syscall extensions is increasing the size of a
struct passed from userspace, such that the zero-value of the new fields
result in the old kernel behaviour (allowing for a mix of userspace and
kernel vintages to operate on one another in most cases).

While this interface exists for communication in both directions, only
one interface is straightforward to have reasonable semantics for
(userspace passing a struct to the kernel). For kernel returns to
userspace, what the correct semantics are (whether there should be an
error if userspace is unaware of a new extension) is very
syscall-dependent and thus probably cannot be unified between syscalls
(a good example of this problem is [1]).

Previously there was no common lib/ function that implemented
the necessary extension-checking semantics (and different syscalls
implemented them slightly differently or incompletely[2]). Future
patches replace common uses of this pattern to make use of
copy_struct_from_user().

[1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
     robustify sched_read_attr() ABI logic and code")

[2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
     similar checks to copy_struct_from_user() while rt_sigprocmask(2)
     always rejects differently-sized struct arguments.

Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 include/linux/uaccess.h |  4 +++
 lib/Makefile            |  2 +-
 lib/strnlen_user.c      | 52 +++++++++++++++++++++++++++++
 lib/struct_user.c       | 73 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 lib/struct_user.c

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index 34a038563d97..824569e309e4 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -230,6 +230,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
 
 #endif		/* ARCH_HAS_NOCACHE_UACCESS */
 
+extern int is_zeroed_user(const void __user *from, size_t count);
+extern int copy_struct_from_user(void *dst, size_t ksize,
+				 const void __user *src, size_t usize);
+
 /*
  * probe_kernel_read(): safely attempt to read from a location
  * @dst: pointer to the buffer that shall take the data
diff --git a/lib/Makefile b/lib/Makefile
index 29c02a924973..d86c71feaf0a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -28,7 +28,7 @@ endif
 CFLAGS_string.o := $(call cc-option, -fno-stack-protector)
 endif
 
-lib-y := ctype.o string.o vsprintf.o cmdline.o \
+lib-y := ctype.o string.o struct_user.o vsprintf.o cmdline.o \
 	 rbtree.o radix-tree.o timerqueue.o xarray.o \
 	 idr.o extable.o \
 	 sha1.o chacha.o irq_regs.o argv_split.o \
diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
index 7f2db3fe311f..7eb665732954 100644
--- a/lib/strnlen_user.c
+++ b/lib/strnlen_user.c
@@ -123,3 +123,55 @@ long strnlen_user(const char __user *str, long count)
 	return 0;
 }
 EXPORT_SYMBOL(strnlen_user);
+
+/**
+ * is_zeroed_user: check if a userspace buffer is full of zeros
+ * @from:  Source address, in userspace.
+ * @size: Size of buffer.
+ *
+ * This is effectively shorthand for "memchr_inv(from, 0, size) == NULL" for
+ * userspace addresses. If there are non-zero bytes present then false is
+ * returned, otherwise true is returned.
+ *
+ * Returns:
+ *  * -EFAULT: access to userspace failed.
+ */
+int is_zeroed_user(const void __user *from, size_t size)
+{
+	u64 val;
+	uintptr_t align = (uintptr_t) from % 8;
+
+	if (unlikely(!size))
+		return true;
+
+	from -= align;
+	size += align;
+
+	if (!user_access_begin(from, size))
+		return -EFAULT;
+
+	while (size >= 8) {
+		unsafe_get_user(val, (u64 __user *) from, err_fault);
+		if (align) {
+			/* @from is unaligned. */
+			val &= ~aligned_byte_mask(align);
+			align = 0;
+		}
+		if (val)
+			goto done;
+		from += 8;
+		size -= 8;
+	}
+	if (size) {
+		/* (@from + @size) is unaligned. */
+		unsafe_get_user(val, (u64 __user *) from, err_fault);
+		val &= aligned_byte_mask(size);
+	}
+
+done:
+	user_access_end();
+	return (val == 0);
+err_fault:
+	user_access_end();
+	return -EFAULT;
+}
diff --git a/lib/struct_user.c b/lib/struct_user.c
new file mode 100644
index 000000000000..57d79eb53bfa
--- /dev/null
+++ b/lib/struct_user.c
@@ -0,0 +1,73 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2019 SUSE LLC
+ * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
+ */
+
+#include <linux/types.h>
+#include <linux/export.h>
+#include <linux/uaccess.h>
+#include <linux/kernel.h>
+#include <linux/string.h>
+
+/**
+ * copy_struct_from_user: copy a struct from userspace
+ * @dst:   Destination address, in kernel space. This buffer must be @ksize
+ *         bytes long.
+ * @ksize: Size of @dst struct.
+ * @src:   Source address, in userspace.
+ * @usize: (Alleged) size of @src struct.
+ *
+ * Copies a struct from userspace to kernel space, in a way that guarantees
+ * backwards-compatibility for struct syscall arguments (as long as future
+ * struct extensions are made such that all new fields are *appended* to the
+ * old struct, and zeroed-out new fields have the same meaning as the old
+ * struct).
+ *
+ * @ksize is just sizeof(*dst), and @usize should've been passed by userspace.
+ * The recommended usage is something like the following:
+ *
+ *   SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
+ *   {
+ *      int err;
+ *      struct foo karg = {};
+ *
+ *      err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
+ *      if (err)
+ *        return err;
+ *
+ *      // ...
+ *   }
+ *
+ * There are three cases to consider:
+ *  * If @usize == @ksize, then it's copied verbatim.
+ *  * If @usize < @ksize, then the userspace has passed an old struct to a
+ *    newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
+ *    are to be zero-filled.
+ *  * If @usize > @ksize, then the userspace has passed a new struct to an
+ *    older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
+ *    are checked to ensure they are zeroed, otherwise -E2BIG is returned.
+ *
+ * Returns (in all cases, some data may have been copied):
+ *  * -E2BIG:  (@usize > @ksize) and there are non-zero trailing bytes in @src.
+ *  * -EFAULT: access to userspace failed.
+ */
+int copy_struct_from_user(void *dst, size_t ksize,
+			  const void __user *src, size_t usize)
+{
+	size_t size = min(ksize, usize);
+	size_t rest = max(ksize, usize) - size;
+
+	/* Deal with trailing bytes. */
+	if (usize < ksize) {
+		memset(dst + size, 0, rest);
+	} else if (usize > ksize) {
+		int ret = is_zeroed_user(src + size, rest);
+		if (ret <= 0)
+			return ret ?: -E2BIG;
+	}
+	/* Copy the interoperable parts of the struct. */
+	if (copy_from_user(dst, src, size))
+		return -EFAULT;
+	return 0;
+}
-- 
2.23.0

[PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[Aleksa Sarai]

The change is very straightforward, and helps unify the syscall
interface for struct-from-userspace syscalls. Additionally, explicitly
define CLONE_ARGS_SIZE_VER0 to match the other users of the
struct-extension pattern.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 include/uapi/linux/sched.h |  2 ++
 kernel/fork.c              | 34 +++++++---------------------------
 2 files changed, 9 insertions(+), 27 deletions(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
index b3105ac1381a..0945805982b4 100644
--- a/include/uapi/linux/sched.h
+++ b/include/uapi/linux/sched.h
@@ -47,6 +47,8 @@ struct clone_args {
 	__aligned_u64 tls;
 };
 
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+
 /*
  * Scheduling policies
  */
diff --git a/kernel/fork.c b/kernel/fork.c
index 541fd805fb88..a86e3841ee4e 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2530,39 +2530,19 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
 #ifdef __ARCH_WANT_SYS_CLONE3
 noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
 					      struct clone_args __user *uargs,
-					      size_t size)
+					      size_t usize)
 {
+	int err;
 	struct clone_args args;
 
-	if (unlikely(size > PAGE_SIZE))
+	if (unlikely(usize > PAGE_SIZE))
 		return -E2BIG;
-
-	if (unlikely(size < sizeof(struct clone_args)))
+	if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
 		return -EINVAL;
 
-	if (unlikely(!access_ok(uargs, size)))
-		return -EFAULT;
-
-	if (size > sizeof(struct clone_args)) {
-		unsigned char __user *addr;
-		unsigned char __user *end;
-		unsigned char val;
-
-		addr = (void __user *)uargs + sizeof(struct clone_args);
-		end = (void __user *)uargs + size;
-
-		for (; addr < end; addr++) {
-			if (get_user(val, addr))
-				return -EFAULT;
-			if (val)
-				return -E2BIG;
-		}
-
-		size = sizeof(struct clone_args);
-	}
-
-	if (copy_from_user(&args, uargs, size))
-		return -EFAULT;
+	err = copy_struct_from_user(&args, sizeof(args), uargs, usize);
+	if (err)
+		return err;
 
 	/*
 	 * Verify that higher 32bits of exit_signal are unset and that
-- 
2.23.0

[PATCH v1 3/4] sched_setattr: switch to copy_struct_from_user()

[Aleksa Sarai]

The change is very straightforward, and helps unify the syscall
interface for struct-from-userspace syscalls. Ideally we could also
unify sched_getattr(2)-style syscalls as well, but unfortunately the
correct semantics for such syscalls are much less clear (see [1] for
more detail). In future we could come up with a more sane idea for how
the syscall interface should look.

[1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
     robustify sched_read_attr() ABI logic and code")

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 kernel/sched/core.c | 43 +++++++------------------------------------
 1 file changed, 7 insertions(+), 36 deletions(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index df9f1fe5689b..cdb2f5e29b88 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -4900,9 +4900,6 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 	u32 size;
 	int ret;
 
-	if (!access_ok(uattr, SCHED_ATTR_SIZE_VER0))
-		return -EFAULT;
-
 	/* Zero the full structure, so that a short copy will be nice: */
 	memset(attr, 0, sizeof(*attr));
 
@@ -4910,45 +4907,19 @@ static int sched_copy_attr(struct sched_attr __user *uattr, struct sched_attr *a
 	if (ret)
 		return ret;
 
-	/* Bail out on silly large: */
-	if (size > PAGE_SIZE)
-		goto err_size;
-
 	/* ABI compatibility quirk: */
 	if (!size)
 		size = SCHED_ATTR_SIZE_VER0;
-
-	if (size < SCHED_ATTR_SIZE_VER0)
+	if (size < SCHED_ATTR_SIZE_VER0 || size > PAGE_SIZE)
 		goto err_size;
 
-	/*
-	 * If we're handed a bigger struct than we know of,
-	 * ensure all the unknown bits are 0 - i.e. new
-	 * user-space does not rely on any kernel feature
-	 * extensions we dont know about yet.
-	 */
-	if (size > sizeof(*attr)) {
-		unsigned char __user *addr;
-		unsigned char __user *end;
-		unsigned char val;
-
-		addr = (void __user *)uattr + sizeof(*attr);
-		end  = (void __user *)uattr + size;
-
-		for (; addr < end; addr++) {
-			ret = get_user(val, addr);
-			if (ret)
-				return ret;
-			if (val)
-				goto err_size;
-		}
-		size = sizeof(*attr);
+	ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
+	if (ret) {
+		if (ret == -E2BIG)
+			goto err_size;
+		return ret;
 	}
 
-	ret = copy_from_user(attr, uattr, size);
-	if (ret)
-		return -EFAULT;
-
 	if ((attr->sched_flags & SCHED_FLAG_UTIL_CLAMP) &&
 	    size < SCHED_ATTR_SIZE_VER1)
 		return -EINVAL;
@@ -5148,7 +5119,7 @@ sched_attr_copy_to_user(struct sched_attr __user *uattr,
  * sys_sched_getattr - similar to sched_getparam, but with sched_attr
  * @pid: the pid in question.
  * @uattr: structure containing the extended parameters.
- * @usize: sizeof(attr) that user-space knows about, for forwards and backwards compatibility.
+ * @usize: sizeof(attr) for fwd/bwd comp.
  * @flags: for future extension.
  */
 SYSCALL_DEFINE4(sched_getattr, pid_t, pid, struct sched_attr __user *, uattr,
-- 
2.23.0

[PATCH v1 4/4] perf_event_open: switch to copy_struct_from_user()

[Aleksa Sarai]

The change is very straightforward, and helps unify the syscall
interface for struct-from-userspace syscalls.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 kernel/events/core.c | 47 +++++++++-----------------------------------
 1 file changed, 9 insertions(+), 38 deletions(-)

diff --git a/kernel/events/core.c b/kernel/events/core.c
index 0463c1151bae..038ed126bc1b 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -10498,55 +10498,26 @@ static int perf_copy_attr(struct perf_event_attr __user *uattr,
 	u32 size;
 	int ret;
 
-	if (!access_ok(uattr, PERF_ATTR_SIZE_VER0))
-		return -EFAULT;
-
-	/*
-	 * zero the full structure, so that a short copy will be nice.
-	 */
+	/* Zero the full structure, so that a short copy will be nice. */
 	memset(attr, 0, sizeof(*attr));
 
 	ret = get_user(size, &uattr->size);
 	if (ret)
 		return ret;
 
-	if (size > PAGE_SIZE)	/* silly large */
-		goto err_size;
-
-	if (!size)		/* abi compat */
+	/* ABI compatibility quirk: */
+	if (!size)
 		size = PERF_ATTR_SIZE_VER0;
-
-	if (size < PERF_ATTR_SIZE_VER0)
+	if (size < PERF_ATTR_SIZE_VER0 || size > PAGE_SIZE)
 		goto err_size;
 
-	/*
-	 * If we're handed a bigger struct than we know of,
-	 * ensure all the unknown bits are 0 - i.e. new
-	 * user-space does not rely on any kernel feature
-	 * extensions we dont know about yet.
-	 */
-	if (size > sizeof(*attr)) {
-		unsigned char __user *addr;
-		unsigned char __user *end;
-		unsigned char val;
-
-		addr = (void __user *)uattr + sizeof(*attr);
-		end  = (void __user *)uattr + size;
-
-		for (; addr < end; addr++) {
-			ret = get_user(val, addr);
-			if (ret)
-				return ret;
-			if (val)
-				goto err_size;
-		}
-		size = sizeof(*attr);
+	ret = copy_struct_from_user(attr, sizeof(*attr), uattr, size);
+	if (ret) {
+		if (ret == -E2BIG)
+			goto err_size;
+		return ret;
 	}
 
-	ret = copy_from_user(attr, uattr, size);
-	if (ret)
-		return -EFAULT;
-
 	attr->size = size;
 
 	if (attr->__reserved_1)
-- 
2.23.0

Re: [PATCH v1 0/4] lib: introduce copy_struct_from_user() helper

[Christian Brauner]

On Wed, Sep 25, 2019 at 06:59:11PM +0200, Aleksa Sarai wrote:
> This series was split off from the openat2(2) syscall discussion[1].
> However, the copy_struct_to_user() helper has been dropped, because
> after some discussion it appears that there is no really obvious
> semantics for how copy_struct_to_user() should work on mixed-vintages
> (for instance, whether [2] is the correct semantics for all syscalls).
> 
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases).
> 
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[3]). This series
> implements the helper and ports several syscalls to use it.
> 
> [1]: https://lore.kernel.org/lkml/20190904201933.10736-1-cyphar@cyphar.com/
> 
> [2]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
>      robustify sched_read_attr() ABI logic and code")
> 
> [3]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
>      similar checks to copy_struct_from_user() while rt_sigprocmask(2)
>      always rejects differently-sized struct arguments.

Thank for splitting this out! :)
I should be able to review this tomorrow.

Christian

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Linus Torvalds]

On Wed, Sep 25, 2019 at 10:00 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> +int is_zeroed_user(const void __user *from, size_t size)

I like how you've done this, but it's buggy and only works on 64-bit.

All the "u64" and "8" cases need to be "unsigned long" and
"sizeof(unsigned long)".

Part of that requirement is:

> +               unsafe_get_user(val, (u64 __user *) from, err_fault);

This part works fine - although 64-bit accesses migth be much more
expensive and the win of unrolling might not be sufficient - but:

> +               if (align) {
> +                       /* @from is unaligned. */
> +                       val &= ~aligned_byte_mask(align);
> +                       align = 0;
> +               }

This part fundamentally only works on 'unsigned long'.

         Linus

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Christian Brauner]

On Wed, Sep 25, 2019 at 06:59:12PM +0200, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases).
> 
> While this interface exists for communication in both directions, only
> one interface is straightforward to have reasonable semantics for
> (userspace passing a struct to the kernel). For kernel returns to
> userspace, what the correct semantics are (whether there should be an
> error if userspace is unaware of a new extension) is very
> syscall-dependent and thus probably cannot be unified between syscalls
> (a good example of this problem is [1]).
> 
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[2]). Future
> patches replace common uses of this pattern to make use of
> copy_struct_from_user().
> 
> [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
>      robustify sched_read_attr() ABI logic and code")
> 
> [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
>      similar checks to copy_struct_from_user() while rt_sigprocmask(2)
>      always rejects differently-sized struct arguments.
> 
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  include/linux/uaccess.h |  4 +++
>  lib/Makefile            |  2 +-
>  lib/strnlen_user.c      | 52 +++++++++++++++++++++++++++++
>  lib/struct_user.c       | 73 +++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 130 insertions(+), 1 deletion(-)
>  create mode 100644 lib/struct_user.c

Hm, why the new file?
Couldn't this just live in usercopy.c?

> 
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index 34a038563d97..824569e309e4 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -230,6 +230,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
>  
>  #endif		/* ARCH_HAS_NOCACHE_UACCESS */
>  
> +extern int is_zeroed_user(const void __user *from, size_t count);
> +extern int copy_struct_from_user(void *dst, size_t ksize,
> +				 const void __user *src, size_t usize);
> +
>  /*
>   * probe_kernel_read(): safely attempt to read from a location
>   * @dst: pointer to the buffer that shall take the data
> diff --git a/lib/Makefile b/lib/Makefile
> index 29c02a924973..d86c71feaf0a 100644
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -28,7 +28,7 @@ endif
>  CFLAGS_string.o := $(call cc-option, -fno-stack-protector)
>  endif
>  
> -lib-y := ctype.o string.o vsprintf.o cmdline.o \
> +lib-y := ctype.o string.o struct_user.o vsprintf.o cmdline.o \
>  	 rbtree.o radix-tree.o timerqueue.o xarray.o \
>  	 idr.o extable.o \
>  	 sha1.o chacha.o irq_regs.o argv_split.o \
> diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> index 7f2db3fe311f..7eb665732954 100644
> --- a/lib/strnlen_user.c
> +++ b/lib/strnlen_user.c
> @@ -123,3 +123,55 @@ long strnlen_user(const char __user *str, long count)
>  	return 0;
>  }
>  EXPORT_SYMBOL(strnlen_user);
> +
> +/**
> + * is_zeroed_user: check if a userspace buffer is full of zeros
> + * @from:  Source address, in userspace.
> + * @size: Size of buffer.
> + *
> + * This is effectively shorthand for "memchr_inv(from, 0, size) == NULL" for
> + * userspace addresses. If there are non-zero bytes present then false is
> + * returned, otherwise true is returned.
> + *
> + * Returns:
> + *  * -EFAULT: access to userspace failed.
> + */
> +int is_zeroed_user(const void __user *from, size_t size)

*sigh*, I'm probably going to get yelled at because of this but: does
this really provide any _performance_ benefits over the dumb get_user()
loop that we currently have that we care about right now? My point
being, that the loop - imho - is much easier to understand than what is
going on here with all the masking, and aligning etc. that we have here.
But I'm not going to fight it.

> +{
> +	u64 val;
> +	uintptr_t align = (uintptr_t) from % 8;
> +
> +	if (unlikely(!size))
> +		return true;

Nit: I'd prefer int variables be checked with if (size != 0) :)

> +
> +	from -= align;
> +	size += align;
> +
> +	if (!user_access_begin(from, size))
> +		return -EFAULT;
> +
> +	while (size >= 8) {
> +		unsafe_get_user(val, (u64 __user *) from, err_fault);
> +		if (align) {
> +			/* @from is unaligned. */
> +			val &= ~aligned_byte_mask(align);
> +			align = 0;
> +		}
> +		if (val)
> +			goto done;
> +		from += 8;
> +		size -= 8;
> +	}
> +	if (size) {
> +		/* (@from + @size) is unaligned. */
> +		unsafe_get_user(val, (u64 __user *) from, err_fault);
> +		val &= aligned_byte_mask(size);
> +	}
> +
> +done:
> +	user_access_end();
> +	return (val == 0);
> +err_fault:
> +	user_access_end();
> +	return -EFAULT;
> +}
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..57d79eb53bfa
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,73 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +/**
> + * copy_struct_from_user: copy a struct from userspace
> + * @dst:   Destination address, in kernel space. This buffer must be @ksize
> + *         bytes long.
> + * @ksize: Size of @dst struct.
> + * @src:   Source address, in userspace.
> + * @usize: (Alleged) size of @src struct.
> + *
> + * Copies a struct from userspace to kernel space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by userspace.
> + * The recommended usage is something like the following:
> + *
> + *   SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> + *   {
> + *      int err;
> + *      struct foo karg = {};
> + *
> + *      err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> + *      if (err)
> + *        return err;
> + *
> + *      // ...
> + *   }
> + *
> + * There are three cases to consider:
> + *  * If @usize == @ksize, then it's copied verbatim.
> + *  * If @usize < @ksize, then the userspace has passed an old struct to a
> + *    newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> + *    are to be zero-filled.
> + *  * If @usize > @ksize, then the userspace has passed a new struct to an
> + *    older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> + *    are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> + *
> + * Returns (in all cases, some data may have been copied):
> + *  * -E2BIG:  (@usize > @ksize) and there are non-zero trailing bytes in @src.
> + *  * -EFAULT: access to userspace failed.
> + */
> +int copy_struct_from_user(void *dst, size_t ksize,
> +			  const void __user *src, size_t usize)
> +{
> +	size_t size = min(ksize, usize);
> +	size_t rest = max(ksize, usize) - size;
> +
> +	/* Deal with trailing bytes. */
> +	if (usize < ksize) {
> +		memset(dst + size, 0, rest);
> +	} else if (usize > ksize) {
> +		int ret = is_zeroed_user(src + size, rest);
> +		if (ret <= 0)
> +			return ret ?: -E2BIG;
> +	}
> +	/* Copy the interoperable parts of the struct. */
> +	if (copy_from_user(dst, src, size))
> +		return -EFAULT;
> +	return 0;
> +}
> -- 
> 2.23.0
> 

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Christian Brauner]

On Wed, Sep 25, 2019 at 07:18:11PM +0200, Christian Brauner wrote:
> On Wed, Sep 25, 2019 at 06:59:12PM +0200, Aleksa Sarai wrote:
> > A common pattern for syscall extensions is increasing the size of a
> > struct passed from userspace, such that the zero-value of the new fields
> > result in the old kernel behaviour (allowing for a mix of userspace and
> > kernel vintages to operate on one another in most cases).
> > 
> > While this interface exists for communication in both directions, only
> > one interface is straightforward to have reasonable semantics for
> > (userspace passing a struct to the kernel). For kernel returns to
> > userspace, what the correct semantics are (whether there should be an
> > error if userspace is unaware of a new extension) is very
> > syscall-dependent and thus probably cannot be unified between syscalls
> > (a good example of this problem is [1]).
> > 
> > Previously there was no common lib/ function that implemented
> > the necessary extension-checking semantics (and different syscalls
> > implemented them slightly differently or incompletely[2]). Future
> > patches replace common uses of this pattern to make use of
> > copy_struct_from_user().
> > 
> > [1]: commit 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and
> >      robustify sched_read_attr() ABI logic and code")
> > 
> > [2]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> >      similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> >      always rejects differently-sized struct arguments.
> > 
> > Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> > Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> > ---
> >  include/linux/uaccess.h |  4 +++
> >  lib/Makefile            |  2 +-
> >  lib/strnlen_user.c      | 52 +++++++++++++++++++++++++++++
> >  lib/struct_user.c       | 73 +++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 130 insertions(+), 1 deletion(-)
> >  create mode 100644 lib/struct_user.c
> 
> Hm, why the new file?
> Couldn't this just live in usercopy.c?
> 
> > 
> > diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> > index 34a038563d97..824569e309e4 100644
> > --- a/include/linux/uaccess.h
> > +++ b/include/linux/uaccess.h
> > @@ -230,6 +230,10 @@ static inline unsigned long __copy_from_user_inatomic_nocache(void *to,
> >  
> >  #endif		/* ARCH_HAS_NOCACHE_UACCESS */
> >  
> > +extern int is_zeroed_user(const void __user *from, size_t count);
> > +extern int copy_struct_from_user(void *dst, size_t ksize,
> > +				 const void __user *src, size_t usize);
> > +
> >  /*
> >   * probe_kernel_read(): safely attempt to read from a location
> >   * @dst: pointer to the buffer that shall take the data
> > diff --git a/lib/Makefile b/lib/Makefile
> > index 29c02a924973..d86c71feaf0a 100644
> > --- a/lib/Makefile
> > +++ b/lib/Makefile
> > @@ -28,7 +28,7 @@ endif
> >  CFLAGS_string.o := $(call cc-option, -fno-stack-protector)
> >  endif
> >  
> > -lib-y := ctype.o string.o vsprintf.o cmdline.o \
> > +lib-y := ctype.o string.o struct_user.o vsprintf.o cmdline.o \
> >  	 rbtree.o radix-tree.o timerqueue.o xarray.o \
> >  	 idr.o extable.o \
> >  	 sha1.o chacha.o irq_regs.o argv_split.o \
> > diff --git a/lib/strnlen_user.c b/lib/strnlen_user.c
> > index 7f2db3fe311f..7eb665732954 100644
> > --- a/lib/strnlen_user.c
> > +++ b/lib/strnlen_user.c
> > @@ -123,3 +123,55 @@ long strnlen_user(const char __user *str, long count)
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL(strnlen_user);
> > +
> > +/**
> > + * is_zeroed_user: check if a userspace buffer is full of zeros
> > + * @from:  Source address, in userspace.
> > + * @size: Size of buffer.
> > + *
> > + * This is effectively shorthand for "memchr_inv(from, 0, size) == NULL" for
> > + * userspace addresses. If there are non-zero bytes present then false is
> > + * returned, otherwise true is returned.
> > + *
> > + * Returns:
> > + *  * -EFAULT: access to userspace failed.
> > + */
> > +int is_zeroed_user(const void __user *from, size_t size)
> 
> *sigh*, I'm probably going to get yelled at because of this but: does
> this really provide any _performance_ benefits over the dumb get_user()
> loop that we currently have that we care about right now? My point
> being, that the loop - imho - is much easier to understand than what is
> going on here with all the masking, and aligning etc. that we have here.
> But I'm not going to fight it.
> 
> > +{
> > +	u64 val;
> > +	uintptr_t align = (uintptr_t) from % 8;
> > +
> > +	if (unlikely(!size))
> > +		return true;
> 
> Nit: I'd prefer int variables be checked with if (size != 0) :)
> 
> > +
> > +	from -= align;
> > +	size += align;
> > +
> > +	if (!user_access_begin(from, size))
> > +		return -EFAULT;
> > +
> > +	while (size >= 8) {
> > +		unsafe_get_user(val, (u64 __user *) from, err_fault);
> > +		if (align) {
> > +			/* @from is unaligned. */
> > +			val &= ~aligned_byte_mask(align);
> > +			align = 0;
> > +		}
> > +		if (val)
> > +			goto done;
> > +		from += 8;
> > +		size -= 8;
> > +	}
> > +	if (size) {
> > +		/* (@from + @size) is unaligned. */
> > +		unsafe_get_user(val, (u64 __user *) from, err_fault);
> > +		val &= aligned_byte_mask(size);
> > +	}
> > +
> > +done:
> > +	user_access_end();
> > +	return (val == 0);
> > +err_fault:
> > +	user_access_end();
> > +	return -EFAULT;
> > +}
> > diff --git a/lib/struct_user.c b/lib/struct_user.c
> > new file mode 100644
> > index 000000000000..57d79eb53bfa
> > --- /dev/null
> > +++ b/lib/struct_user.c
> > @@ -0,0 +1,73 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Copyright (C) 2019 SUSE LLC
> > + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> > + */
> > +
> > +#include <linux/types.h>
> > +#include <linux/export.h>
> > +#include <linux/uaccess.h>
> > +#include <linux/kernel.h>
> > +#include <linux/string.h>
> > +
> > +/**
> > + * copy_struct_from_user: copy a struct from userspace
> > + * @dst:   Destination address, in kernel space. This buffer must be @ksize
> > + *         bytes long.
> > + * @ksize: Size of @dst struct.
> > + * @src:   Source address, in userspace.
> > + * @usize: (Alleged) size of @src struct.
> > + *
> > + * Copies a struct from userspace to kernel space, in a way that guarantees
> > + * backwards-compatibility for struct syscall arguments (as long as future
> > + * struct extensions are made such that all new fields are *appended* to the
> > + * old struct, and zeroed-out new fields have the same meaning as the old
> > + * struct).
> > + *
> > + * @ksize is just sizeof(*dst), and @usize should've been passed by userspace.
> > + * The recommended usage is something like the following:
> > + *
> > + *   SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> > + *   {
> > + *      int err;
> > + *      struct foo karg = {};
> > + *
> > + *      err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> > + *      if (err)
> > + *        return err;
> > + *
> > + *      // ...
> > + *   }
> > + *
> > + * There are three cases to consider:
> > + *  * If @usize == @ksize, then it's copied verbatim.
> > + *  * If @usize < @ksize, then the userspace has passed an old struct to a
> > + *    newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> > + *    are to be zero-filled.
> > + *  * If @usize > @ksize, then the userspace has passed a new struct to an
> > + *    older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> > + *    are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> > + *
> > + * Returns (in all cases, some data may have been copied):
> > + *  * -E2BIG:  (@usize > @ksize) and there are non-zero trailing bytes in @src.
> > + *  * -EFAULT: access to userspace failed.
> > + */
> > +int copy_struct_from_user(void *dst, size_t ksize,
> > +			  const void __user *src, size_t usize)

Hm, and should that get tests in test_usercopy.c?

Christian

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Aleksa Sarai]

[-- Attachment #1: Type: text/plain, Size: 2676 bytes --]

On 2019-09-25, Linus Torvalds <torvalds@linux-foundation.org> wrote:
> On Wed, Sep 25, 2019 at 10:00 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > +int is_zeroed_user(const void __user *from, size_t size)
> 
> I like how you've done this, but it's buggy and only works on 64-bit.
> 
> All the "u64" and "8" cases need to be "unsigned long" and
> "sizeof(unsigned long)".
> 
> Part of that requirement is:
> 
> > +               unsafe_get_user(val, (u64 __user *) from, err_fault);
> 
> This part works fine - although 64-bit accesses migth be much more
> expensive and the win of unrolling might not be sufficient - but:
> 
> > +               if (align) {
> > +                       /* @from is unaligned. */
> > +                       val &= ~aligned_byte_mask(align);
> > +                       align = 0;
> > +               }
> 
> This part fundamentally only works on 'unsigned long'.

Just to make sure I understand, the following diff would this solve the
problem? If so, I'll apply it, and re-send in a few hours.

--8<--------------------------------------------------------------------------

 int is_zeroed_user(const void __user *from, size_t size)
 {
-       u64 val;
-       uintptr_t align = (uintptr_t) from % 8;
+       unsigned long val;
+       uintptr_t align = (uintptr_t) from % sizeof(unsigned long);
 
        if (unlikely(!size))
                return true;
@@ -150,8 +150,8 @@ int is_zeroed_user(const void __user *from, size_t size)
        if (!user_access_begin(from, size))
                return -EFAULT;
 
-       while (size >= 8) {
-               unsafe_get_user(val, (u64 __user *) from, err_fault);
+       while (size >= sizeof(unsigned long)) {
+               unsafe_get_user(val, (unsigned long __user *) from, err_fault);
                if (align) {
                        /* @from is unaligned. */
                        val &= ~aligned_byte_mask(align);
@@ -159,12 +159,12 @@ int is_zeroed_user(const void __user *from, size_t size)
                }
                if (val)
                        goto done;
-               from += 8;
-               size -= 8;
+               from += sizeof(unsigned long);
+               size -= sizeof(unsigned long);
        }
        if (size) {
                /* (@from + @size) is unaligned. */
-               unsafe_get_user(val, (u64 __user *) from, err_fault);
+               unsafe_get_user(val, (unsigned long __user *) from, err_fault);
                val &= aligned_byte_mask(size);
        }

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[Christian Brauner]

On Wed, Sep 25, 2019 at 06:59:13PM +0200, Aleksa Sarai wrote:
> The change is very straightforward, and helps unify the syscall
> interface for struct-from-userspace syscalls. Additionally, explicitly
> define CLONE_ARGS_SIZE_VER0 to match the other users of the
> struct-extension pattern.
> 
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
>  include/uapi/linux/sched.h |  2 ++
>  kernel/fork.c              | 34 +++++++---------------------------
>  2 files changed, 9 insertions(+), 27 deletions(-)
> 
> diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h
> index b3105ac1381a..0945805982b4 100644
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -47,6 +47,8 @@ struct clone_args {
>  	__aligned_u64 tls;
>  };
>  
> +#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
> +
>  /*
>   * Scheduling policies
>   */
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 541fd805fb88..a86e3841ee4e 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2530,39 +2530,19 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, unsigned long, newsp,
>  #ifdef __ARCH_WANT_SYS_CLONE3
>  noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs,
>  					      struct clone_args __user *uargs,
> -					      size_t size)
> +					      size_t usize)
>  {
> +	int err;
>  	struct clone_args args;
>  
> -	if (unlikely(size > PAGE_SIZE))
> +	if (unlikely(usize > PAGE_SIZE))
>  		return -E2BIG;
> -
> -	if (unlikely(size < sizeof(struct clone_args)))
> +	if (unlikely(usize < CLONE_ARGS_SIZE_VER0))
>  		return -EINVAL;

You might want to rebase this patchset after the merge window closes on
rc1 since that code has changed right before the 5.3 release. But if you
can't don't worry I can also fix it up.

Christian

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Linus Torvalds]

On Wed, Sep 25, 2019 at 10:21 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
>
> Just to make sure I understand, the following diff would this solve the
> problem? If so, I'll apply it, and re-send in a few hours.

Actually, looking at it more, it's still buggy.

That final "size smaller than unsigned long" doesn't correctly handle
the case of (say) a single byte in the middle of a 8-byte word.

So you need to do something like this:

    int is_zeroed_user(const void __user *from, size_t size)
  {
        unsigned long val, mask, align;

        if (unlikely(!size))
                return true;

        if (!user_access_begin(from, size))
                return -EFAULT;

        align = (uintptr_t) from % sizeof(unsigned long);
        from -= align;
        size += align;

        mask = ~aligned_byte_mask(align);

        while (size >= sizeof(unsigned long)) {
                unsafe_get_user(val, (unsigned long __user *) from, err_fault);
                val &= mask;
                if (unlikely(val))
                        goto done;
                mask = ~0ul;
                from += sizeof(unsigned long);
                size -= sizeof(unsigned long);
        }

        if (size) {
                /* (@from + @size) is unaligned. */
                unsafe_get_user(val, (unsigned long __user *) from, err_fault);
                mask &= aligned_byte_mask(size);
                val &= mask;
        }

  done:
        user_access_end();
        return (val == 0);
  err_fault:
        user_access_end();
        return -EFAULT;
  }

note how "mask" carries around from the very beginning all the way to
the end, and "align" itself is no longer used after mask has been
calculated.

That's required because of say a 2-byte read at offset 5. You end up
with "align=5, size=7" at the beginning, and mask needs to be
0x00ffff0000000000 (on little-endian) for that final access.

Anyway, I checked, and the above seems to generate ok code quality
too. Sadly "unsafe_get_user()" cannot use "asm goto" because of a gcc
limitation (no asm goto with outputs), so it's not _perfect_, but
that's literally a compiler limitation.

But I didn't actually _test_ the end result. You should probably
verify that it gets the right behavior exactly for those interesting
cases where we mask both the beginning and the end.

            Linus

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Al Viro]

On Wed, Sep 25, 2019 at 10:48:31AM -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2019 at 10:21 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> >
> > Just to make sure I understand, the following diff would this solve the
> > problem? If so, I'll apply it, and re-send in a few hours.
> 
> Actually, looking at it more, it's still buggy.
> 
> That final "size smaller than unsigned long" doesn't correctly handle
> the case of (say) a single byte in the middle of a 8-byte word.
> 
> So you need to do something like this:
> 
>     int is_zeroed_user(const void __user *from, size_t size)
>   {
>         unsigned long val, mask, align;
> 
>         if (unlikely(!size))
>                 return true;
> 
>         if (!user_access_begin(from, size))
>                 return -EFAULT;
> 
>         align = (uintptr_t) from % sizeof(unsigned long);
>         from -= align;
>         size += align;
> 
>         mask = ~aligned_byte_mask(align);
> 
>         while (size >= sizeof(unsigned long)) {
>                 unsafe_get_user(val, (unsigned long __user *) from, err_fault);
>                 val &= mask;
>                 if (unlikely(val))
>                         goto done;
>                 mask = ~0ul;
>                 from += sizeof(unsigned long);
>                 size -= sizeof(unsigned long);
>         }
> 
>         if (size) {
>                 /* (@from + @size) is unaligned. */
>                 unsafe_get_user(val, (unsigned long __user *) from, err_fault);
>                 mask &= aligned_byte_mask(size);
>                 val &= mask;
>         }

IMO it's better to lift reading the first word out of the loop, like this:

	align = (uintptr_t) from % sizeof(unsigned long);
	from -= align;

	unsafe_get_user(val, (unsigned long __user *) from, err_fault);
	if (align) {
		size += align;
		val &= ~aligned_byte_mask(align);
	}

	while (size > sizeof(unsigned long)) {
		if (unlikely(val))
			goto done;
		from += sizeof(unsigned long);
		size -= sizeof(unsigned long);
		unsafe_get_user(val, (unsigned long __user *) from, err_fault);
	}

	if (size != size(unsigned long))
		val &= aligned_byte_mask(size);
done:

Do you see any problems with that variant?

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Linus Torvalds]

On Wed, Sep 25, 2019 at 11:04 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> IMO it's better to lift reading the first word out of the loop, like this:
>   ...
> Do you see any problems with that variant?

That looks fine to me too.

It's a bit harder for humans to read because of how it reads the word
from user space one iteration early, but from a code generation
standpoint it probably is better.

So slightly subtler source code, and imho harder to read, but it looks
correct to me.

             Linus

Re: [PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: xtensa-common_defconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> lib/struct_user.o:(.text+0x8): undefined reference to `is_zeroed_user'
   lib/struct_user.o: In function `copy_struct_from_user':
   struct_user.c:(.text+0x37): undefined reference to `is_zeroed_user'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10460 bytes --]

Re: [PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1351 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: xtensa-common_defconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=xtensa 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> lib/struct_user.o:(.text+0x8): undefined reference to `is_zeroed_user'
   lib/struct_user.o: In function `copy_struct_from_user':
   struct_user.c:(.text+0x37): undefined reference to `is_zeroed_user'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 10460 bytes --]

Re: [PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: parisc-b180_defconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/struct_user.o: In function `copy_struct_from_user':
>> (.text+0x84): undefined reference to `is_zeroed_user'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14071 bytes --]

Re: [PATCH v1 2/4] clone3: switch to copy_struct_from_user()

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1259 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: parisc-b180_defconfig (attached as .config)
compiler: hppa-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=parisc 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/struct_user.o: In function `copy_struct_from_user':
>> (.text+0x84): undefined reference to `is_zeroed_user'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 14071 bytes --]

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/strnlen_user.o: In function `is_zeroed_user':
>> strnlen_user.c:(.text+0x1a8): undefined reference to `__get_user_bad'
   strnlen_user.c:(.text+0x204): undefined reference to `__get_user_bad'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26190 bytes --]

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1346 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: arm-at91_dt_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=arm 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/strnlen_user.o: In function `is_zeroed_user':
>> strnlen_user.c:(.text+0x1a8): undefined reference to `__get_user_bad'
   strnlen_user.c:(.text+0x204): undefined reference to `__get_user_bad'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 26190 bytes --]

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Al Viro]

On Wed, Sep 25, 2019 at 11:13:18AM -0700, Linus Torvalds wrote:
> On Wed, Sep 25, 2019 at 11:04 AM Al Viro <viro@zeniv.linux.org.uk> wrote:
> >
> > IMO it's better to lift reading the first word out of the loop, like this:
> >   ...
> > Do you see any problems with that variant?
> 
> That looks fine to me too.
> 
> It's a bit harder for humans to read because of how it reads the word
> from user space one iteration early, but from a code generation
> standpoint it probably is better.
> 
> So slightly subtler source code, and imho harder to read, but it looks
> correct to me.

FWIW, I would probably add a kernel-space analogue of that thing at the
same time - check that an area is all-zeroes is not all that rare.
In fact, most of the callers of memchr_inv() are exactly that:

arch/x86/kernel/fpu/xstate.c:492:       if (memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
crypto/async_tx/async_xor.c:225:        return !memchr_inv(page_address(p) + offset, 0, len);
crypto/dh_helper.c:109: if (memchr_inv(params->p, 0, params->p_size) == NULL)
crypto/testmgr.c:446:           memchr_inv(&divs[i], 0, (count - i) * sizeof(divs[0])) == NULL;
crypto/testmgr.c:470:           if (memchr_inv(cfg->dst_divs, 0, sizeof(cfg->dst_divs))) 
crypto/testmgr.c:3802:  if (memchr_inv(outbuf_dec, 0, out_len - m_size) ||
drivers/block/rbd.c:3138:               if (memchr_inv(page_address(bv.bv_page) + bv.bv_offset, 0, 
drivers/gpu/drm/drm_dp_mst_topology.c:1808:     if (memchr_inv(guid, 0, 16))
drivers/gpu/drm/drm_edid.c:1359:        if (memchr_inv(in_edid, 0, length))
drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:238:       if (memchr_inv(ptr, 0, dmabuf->size)) {
drivers/gpu/drm/i915/gem/selftests/i915_gem_dmabuf.c:284:       if (memchr_inv(ptr, 0, PAGE_SIZE)) {
drivers/infiniband/core/uverbs_cmd.c:2741:              if (memchr_inv(kern_spec_filter +
drivers/infiniband/core/uverbs_ioctl.c:143:     return !memchr_inv((const void *)&uattr->data + len,
drivers/infiniband/hw/efa/efa_verbs.c:127:      !memchr_inv(reserved, 0, sizeof(reserved))
drivers/infiniband/hw/mlx4/main.c:1330: memchr_inv((void *)&filter.field  +\
drivers/infiniband/hw/mlx4/qp.c:728:    if (memchr_inv(ucmd.reserved, 0, sizeof(ucmd.reserved)))
drivers/infiniband/hw/mlx5/main.c:2516: !(memchr_inv(MLX5_ADDR_OF(fte_match_param, match_criteria, headers), \
drivers/infiniband/hw/mlx5/main.c:2621: memchr_inv((void *)&filter.field  +\ 
drivers/infiniband/hw/mlx5/qp.c:3921:               memchr_inv(&ucmd.reserved, 0, sizeof(ucmd.reserved)) ||
drivers/infiniband/hw/mlx5/qp.c:3922:               memchr_inv(&ucmd.burst_info.reserved, 0,
drivers/md/dm-integrity.c:3844:                 if (memchr_inv(ic->sb, 0, SB_SECTORS << SECTOR_SHIFT)) {
drivers/mtd/nand/raw/qcom_nandc.c:1548: if (memchr_inv(data_buf, 0xff, data_len)) {
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c:179: if (memchr_inv(&enc_opts.mask->data, 0, sizeof(enc_opts.mask->data)) &&
drivers/net/ethernet/mellanox/mlx5/core/en/tc_tun_geneve.c:240: if (!memchr_inv(option_key->opt_data, 0, option_key->length * 4)) {
drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c:113:         if (memchr_inv(misc, 0, MLX5_ST_SZ_BYTES(fte_match_set_misc)))
drivers/net/ethernet/mellanox/mlxsw/core_acl_flex_keys.c:374:   if (!memchr_inv(mask_value, 0, len)) /* If mask is zero */
drivers/net/geneve.c:1243:               memchr_inv(&info->key.u, 0, sizeof(info->key.u)));
drivers/nvme/host/core.c:1619:          memchr_inv(ids->nguid, 0, sizeof(ids->nguid)) ||
drivers/nvme/host/core.c:1620:          memchr_inv(ids->eui64, 0, sizeof(ids->eui64));
drivers/nvme/host/core.c:2884:  if (memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
drivers/nvme/host/core.c:2887:  if (memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
drivers/nvme/host/core.c:2962:              !memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
drivers/nvme/host/core.c:2966:          if (!memchr_inv(ids->nguid, 0, sizeof(ids->nguid)))
drivers/nvme/host/core.c:2970:          if (!memchr_inv(ids->eui64, 0, sizeof(ids->eui64)))
drivers/nvme/target/admin-cmd.c:544:    if (memchr_inv(&ns->uuid, 0, sizeof(ns->uuid))) {
drivers/nvme/target/admin-cmd.c:551:    if (memchr_inv(ns->nguid, 0, sizeof(ns->nguid))) {
drivers/target/target_core_iblock.c:425:        not_zero = memchr_inv(buf, 0x00, cmd->data_length);
fs/btrfs/ioctl.c:4586:          if (memchr_inv(loi->reserved, 0, sizeof(loi->reserved))) {
fs/cramfs/inode.c:351:  return memchr_inv(tail_data, 0, PAGE_SIZE - partial) ? true : false;
fs/ext4/ioctl.c:649:    if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
fs/ext4/ioctl.c:650:        memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
fs/ext4/ioctl.c:652:        memchr_inv(head.fmh_keys[1].fmr_reserved, 0,
fs/gfs2/lock_dlm.c:485: return !memchr_inv(lvb + JID_BITMAP_OFFSET, 0,
fs/ubifs/auth.c:565:    return !memchr_inv(hmac, 0, c->hmac_desc_len);
fs/xfs/scrub/agheader.c:276:            if (memchr_inv(&sb->sb_features_compat, 0,
fs/xfs/scrub/agheader.c:332:    if (memchr_inv(sb + 1, 0,
fs/xfs/scrub/scrub.c:371:       if (memchr_inv(sm->sm_reserved, 0, sizeof(sm->sm_reserved)))
fs/xfs/xfs_icache.h:95: if (memchr_inv(&src->pad32, 0, sizeof(src->pad32)) ||
fs/xfs/xfs_icache.h:96:     memchr_inv(src->pad64, 0, sizeof(src->pad64)))
fs/xfs/xfs_ioctl.c:846:     memchr_inv(hdr->reserved, 0, sizeof(hdr->reserved)))
fs/xfs/xfs_ioctl.c:1866:        if (memchr_inv(head.fmh_reserved, 0, sizeof(head.fmh_reserved)) ||
fs/xfs/xfs_ioctl.c:1867:            memchr_inv(head.fmh_keys[0].fmr_reserved, 0,
fs/xfs/xfs_ioctl.c:1869:            memchr_inv(head.fmh_keys[1].fmr_reserved, 0,
include/rdma/ib_verbs.h:2782:   ret = !memchr_inv(buf, 0, len);
kernel/bpf/syscall.c:461:       memchr_inv((void *) &attr->CMD##_LAST_FIELD + \
mm/vmstat.c:1827:               if (memchr_inv(p->vm_stat_diff, 0, NR_VM_ZONE_STAT_ITEMS *
mm/vmstat.c:1831:               if (memchr_inv(p->vm_numa_stat_diff, 0, NR_VM_NUMA_STAT_ITEMS *
net/bpf/test_run.c:196: return !memchr_inv((u8 *)buf + from, 0, to - from);
net/sched/act_ct.c:317: if (!memchr_inv(labels_m, 0, labels_sz))
net/sched/act_ct.c:762: if (mask && !memchr_inv(mask, 0, len))
net/sched/cls_flower.c:1322:    memchr_inv(((char *)mask) + FL_KEY_MEMBER_OFFSET(member),               \
net/sched/cls_flower.c:1967:    if (!memchr_inv(mask, 0, len))
net/sched/cls_flower.c:2006:    if (!memchr_inv(mpls_mask, 0, sizeof(*mpls_mask)))
net/sched/cls_flower.c:2058:    if (!memchr_inv(vlan_mask, 0, sizeof(*vlan_mask)))
net/sched/cls_flower.c:2092:    if (!memchr_inv(&flags_mask, 0, sizeof(flags_mask)))
security/keys/dh.c:257:         if (memchr_inv(kdfcopy->__spare, 0, sizeof(kdfcopy->__spare))) {

- 66 out of 90.  So it smell like we might want something like
bool all_zeroes(const void *p, size_t)
somewhere in lib/string.c, with comment regarding keeping it in sync
with __user variant.

Another thing is that for s390 we almost certainly want something better
than word-by-word.  IIRC, word-sized userland accesses really hurt there.
It's nowhere near as critical as with strncpy_from_user(), but with the
same underlying issue.

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[Linus Torvalds]

On Wed, Sep 25, 2019 at 12:43 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> FWIW, I would probably add a kernel-space analogue of that thing at the
> same time - check that an area is all-zeroes is not all that rare.

Hmm. Maybe.

> Another thing is that for s390 we almost certainly want something better
> than word-by-word.  IIRC, word-sized userland accesses really hurt there.
> It's nowhere near as critical as with strncpy_from_user(), but with the
> same underlying issue.

Well, s390 does have those magic "area" instructions, but part of why
it's expensive on s390 is that they haven't implemented the
"user_access_begin()/end()' stuff. I think s390 could use that to at
least minimize some of the costs.

With the common case presumably being just a couple of words, it migth
not be worth it doing anything more than that even on s390.

Interestingly (or perhaps not) if I read the internal s390
implementation correctly, they kind of _have_ that concept and they
use it internally. It's just that they call it "enable_sacf_uaccess()"
and "disable_sacf_uaccess()" instead.

           Linus

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: sh-rsk7269_defconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/strnlen_user.o: In function `is_zeroed_user':
>> strnlen_user.c:(.text+0x160): undefined reference to `__get_user_unknown'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11573 bytes --]

Re: [PATCH v1 1/4] lib: introduce copy_struct_from_user() helper

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1267 bytes --]

Hi Aleksa,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[cannot apply to v5.3 next-20190924]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Aleksa-Sarai/lib-introduce-copy_struct_from_user-helper/20190926-010527
config: sh-rsk7269_defconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 7.4.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=7.4.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   lib/strnlen_user.o: In function `is_zeroed_user':
>> strnlen_user.c:(.text+0x160): undefined reference to `__get_user_unknown'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 11573 bytes --]