Merge tag 'copy-struct-from-user-v5.4-rc2' of git://git.kernel.org/pub/scm/linux...
authorLinus Torvalds <torvalds@linux-foundation.org>
Fri, 4 Oct 2019 17:36:31 +0000 (10:36 -0700)
committerLinus Torvalds <torvalds@linux-foundation.org>
Fri, 4 Oct 2019 17:36:31 +0000 (10:36 -0700)
Pull copy_struct_from_user() helper from Christian Brauner:
 "This contains the copy_struct_from_user() helper which got split out
  from the openat2() patchset. It is a generic interface designed to
  copy a struct from userspace.

  The helper will be especially useful for structs versioned by size of
  which we have quite a few. This allows for backwards compatibility,
  i.e. an extended struct can be passed to an older kernel, or a legacy
  struct can be passed to a newer kernel. For the first case (extended
  struct, older kernel) the new fields in an extended struct can be set
  to zero and the struct safely passed to an older kernel.

  The most obvious benefit is that this helper lets us get rid of
  duplicate code present in at least sched_setattr(), perf_event_open(),
  and clone3(). More importantly it will also help to ensure that users
  implementing versioning-by-size end up with the same core semantics.

  This point is especially crucial since we have at least one case where
  versioning-by-size is used but with slighly different semantics:
  sched_setattr(), perf_event_open(), and clone3() all do do similar
  checks to copy_struct_from_user() while rt_sigprocmask(2) always
  rejects differently-sized struct arguments.

  With this pull request we also switch over sched_setattr(),
  perf_event_open(), and clone3() to use the new helper"

* tag 'copy-struct-from-user-v5.4-rc2' of git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux:
  usercopy: Add parentheses around assignment in test_copy_struct_from_user
  perf_event_open: switch to copy_struct_from_user()
  sched_setattr: switch to copy_struct_from_user()
  clone3: switch to copy_struct_from_user()
  lib: introduce copy_struct_from_user() helper

include/linux/bitops.h
include/linux/uaccess.h
include/uapi/linux/sched.h
kernel/events/core.c
kernel/fork.c
kernel/sched/core.c
lib/strnlen_user.c
lib/test_user_copy.c
lib/usercopy.c

index cf074bce3eb322c2ced7e3e05641f4c87d080ee0..c94a9ff9f082e3301809eb5f47c0493bdbaddc7e 100644 (file)
@@ -4,6 +4,13 @@
 #include <asm/types.h>
 #include <linux/bits.h>
 
+/* Set bits in the first 'n' bytes when loaded from memory */
+#ifdef __LITTLE_ENDIAN
+#  define aligned_byte_mask(n) ((1UL << 8*(n))-1)
+#else
+#  define aligned_byte_mask(n) (~0xffUL << (BITS_PER_LONG - 8 - 8*(n)))
+#endif
+
 #define BITS_PER_TYPE(type) (sizeof(type) * BITS_PER_BYTE)
 #define BITS_TO_LONGS(nr)      DIV_ROUND_UP(nr, BITS_PER_TYPE(long))
 
index 70bbdc38dc37d3cafe82df3b140df557d3b6d7a9..e47d0522a1f47ef70ec071e0ebd98178563cc11c 100644 (file)
@@ -231,6 +231,76 @@ __copy_from_user_inatomic_nocache(void *to, const void __user *from,
 
 #endif         /* ARCH_HAS_NOCACHE_UACCESS */
 
+extern __must_check int check_zeroed_user(const void __user *from, size_t size);
+
+/**
+ * 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 = {};
+ *
+ *      if (usize > PAGE_SIZE)
+ *        return -E2BIG;
+ *      if (usize < FOO_SIZE_VER0)
+ *        return -EINVAL;
+ *
+ *      err = copy_struct_from_user(&karg, sizeof(karg), uarg, usize);
+ *      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.
+ */
+static __always_inline __must_check 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 = check_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;
+}
+
 /*
  * probe_kernel_read(): safely attempt to read from a location
  * @dst: pointer to the buffer that shall take the data
index 3d8f03bfd428e283178e31ef195498e6c578e467..99335e1f4a275b1dbc86219a6dee7701b18fd624 100644 (file)
@@ -71,6 +71,8 @@ struct clone_args {
 };
 #endif
 
+#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */
+
 /*
  * Scheduling policies
  */
index 4655adbbae10d09eabfe155744b4e8539c507494..3f0cb82e4fbcc562e76d2d5d2df649860c07b304 100644 (file)
@@ -10586,55 +10586,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)
index bf11cf39579ae50d88ff2c9f545fa6f249b9e72e..1f6c45f6a734dee95199e984bee662e884729a81 100644 (file)
@@ -2525,39 +2525,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
index 7880f4f64d0eea19dab03a0408625a505b4454ed..dd05a378631a866c2f499c56986e5de7a703e766 100644 (file)
@@ -5106,9 +5106,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));
 
@@ -5116,45 +5113,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;
@@ -5354,7 +5325,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,
index 28ff554a1be867fde4da4ecb2cd1631633f75f34..6c0005d5dd5c43e7ed4097c24d5fc785a794110e 100644 (file)
@@ -3,16 +3,10 @@
 #include <linux/export.h>
 #include <linux/uaccess.h>
 #include <linux/mm.h>
+#include <linux/bitops.h>
 
 #include <asm/word-at-a-time.h>
 
-/* Set bits in the first 'n' bytes when loaded from memory */
-#ifdef __LITTLE_ENDIAN
-#  define aligned_byte_mask(n) ((1ul << 8*(n))-1)
-#else
-#  define aligned_byte_mask(n) (~0xfful << (BITS_PER_LONG - 8 - 8*(n)))
-#endif
-
 /*
  * Do a strnlen, return length of string *with* final '\0'.
  * 'count' is the user-supplied count, while 'max' is the
index 67bcd5dfd847a651150f2f52d6c69f561c15f557..e365ace06538362abf41fce70273fcb32a2d1f18 100644 (file)
 # define TEST_U64
 #endif
 
-#define test(condition, msg)           \
-({                                     \
-       int cond = (condition);         \
-       if (cond)                       \
-               pr_warn("%s\n", msg);   \
-       cond;                           \
+#define test(condition, msg, ...)                                      \
+({                                                                     \
+       int cond = (condition);                                         \
+       if (cond)                                                       \
+               pr_warn("[%d] " msg "\n", __LINE__, ##__VA_ARGS__);     \
+       cond;                                                           \
 })
 
+static bool is_zeroed(void *from, size_t size)
+{
+       return memchr_inv(from, 0x0, size) == NULL;
+}
+
+static int test_check_nonzero_user(char *kmem, char __user *umem, size_t size)
+{
+       int ret = 0;
+       size_t start, end, i;
+       size_t zero_start = size / 4;
+       size_t zero_end = size - zero_start;
+
+       /*
+        * We conduct a series of check_nonzero_user() tests on a block of memory
+        * with the following byte-pattern (trying every possible [start,end]
+        * pair):
+        *
+        *   [ 00 ff 00 ff ... 00 00 00 00 ... ff 00 ff 00 ]
+        *
+        * And we verify that check_nonzero_user() acts identically to memchr_inv().
+        */
+
+       memset(kmem, 0x0, size);
+       for (i = 1; i < zero_start; i += 2)
+               kmem[i] = 0xff;
+       for (i = zero_end; i < size; i += 2)
+               kmem[i] = 0xff;
+
+       ret |= test(copy_to_user(umem, kmem, size),
+                   "legitimate copy_to_user failed");
+
+       for (start = 0; start <= size; start++) {
+               for (end = start; end <= size; end++) {
+                       size_t len = end - start;
+                       int retval = check_zeroed_user(umem + start, len);
+                       int expected = is_zeroed(kmem + start, len);
+
+                       ret |= test(retval != expected,
+                                   "check_nonzero_user(=%d) != memchr_inv(=%d) mismatch (start=%zu, end=%zu)",
+                                   retval, expected, start, end);
+               }
+       }
+
+       return ret;
+}
+
+static int test_copy_struct_from_user(char *kmem, char __user *umem,
+                                     size_t size)
+{
+       int ret = 0;
+       char *umem_src = NULL, *expected = NULL;
+       size_t ksize, usize;
+
+       umem_src = kmalloc(size, GFP_KERNEL);
+       if ((ret |= test(umem_src == NULL, "kmalloc failed")))
+               goto out_free;
+
+       expected = kmalloc(size, GFP_KERNEL);
+       if ((ret |= test(expected == NULL, "kmalloc failed")))
+               goto out_free;
+
+       /* Fill umem with a fixed byte pattern. */
+       memset(umem_src, 0x3e, size);
+       ret |= test(copy_to_user(umem, umem_src, size),
+                   "legitimate copy_to_user failed");
+
+       /* Check basic case -- (usize == ksize). */
+       ksize = size;
+       usize = size;
+
+       memcpy(expected, umem_src, ksize);
+
+       memset(kmem, 0x0, size);
+       ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+                   "copy_struct_from_user(usize == ksize) failed");
+       ret |= test(memcmp(kmem, expected, ksize),
+                   "copy_struct_from_user(usize == ksize) gives unexpected copy");
+
+       /* Old userspace case -- (usize < ksize). */
+       ksize = size;
+       usize = size / 2;
+
+       memcpy(expected, umem_src, usize);
+       memset(expected + usize, 0x0, ksize - usize);
+
+       memset(kmem, 0x0, size);
+       ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+                   "copy_struct_from_user(usize < ksize) failed");
+       ret |= test(memcmp(kmem, expected, ksize),
+                   "copy_struct_from_user(usize < ksize) gives unexpected copy");
+
+       /* New userspace (-E2BIG) case -- (usize > ksize). */
+       ksize = size / 2;
+       usize = size;
+
+       memset(kmem, 0x0, size);
+       ret |= test(copy_struct_from_user(kmem, ksize, umem, usize) != -E2BIG,
+                   "copy_struct_from_user(usize > ksize) didn't give E2BIG");
+
+       /* New userspace (success) case -- (usize > ksize). */
+       ksize = size / 2;
+       usize = size;
+
+       memcpy(expected, umem_src, ksize);
+       ret |= test(clear_user(umem + ksize, usize - ksize),
+                   "legitimate clear_user failed");
+
+       memset(kmem, 0x0, size);
+       ret |= test(copy_struct_from_user(kmem, ksize, umem, usize),
+                   "copy_struct_from_user(usize > ksize) failed");
+       ret |= test(memcmp(kmem, expected, ksize),
+                   "copy_struct_from_user(usize > ksize) gives unexpected copy");
+
+out_free:
+       kfree(expected);
+       kfree(umem_src);
+       return ret;
+}
+
 static int __init test_user_copy_init(void)
 {
        int ret = 0;
@@ -106,6 +225,11 @@ static int __init test_user_copy_init(void)
 #endif
 #undef test_legit
 
+       /* Test usage of check_nonzero_user(). */
+       ret |= test_check_nonzero_user(kmem, usermem, 2 * PAGE_SIZE);
+       /* Test usage of copy_struct_from_user(). */
+       ret |= test_copy_struct_from_user(kmem, usermem, 2 * PAGE_SIZE);
+
        /*
         * Invalid usage: none of these copies should succeed.
         */
index c2bfbcaeb3dc5bb9dc2a1efd4b30acd2e0ad6f90..cbb4d9ec00f207a8156cab8a4aa52fb8a49ff43e 100644 (file)
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <linux/uaccess.h>
+#include <linux/bitops.h>
 
 /* out-of-line parts */
 
@@ -31,3 +32,57 @@ unsigned long _copy_to_user(void __user *to, const void *from, unsigned long n)
 }
 EXPORT_SYMBOL(_copy_to_user);
 #endif
+
+/**
+ * check_zeroed_user: check if a userspace buffer only contains zero bytes
+ * @from: Source address, in userspace.
+ * @size: Size of buffer.
+ *
+ * This is effectively shorthand for "memchr_inv(from, 0, size) == NULL" for
+ * userspace addresses (and is more efficient because we don't care where the
+ * first non-zero byte is).
+ *
+ * Returns:
+ *  * 0: There were non-zero bytes present in the buffer.
+ *  * 1: The buffer was full of zero bytes.
+ *  * -EFAULT: access to userspace failed.
+ */
+int check_zeroed_user(const void __user *from, size_t size)
+{
+       unsigned long val;
+       uintptr_t align = (uintptr_t) from % sizeof(unsigned long);
+
+       if (unlikely(size == 0))
+               return 1;
+
+       from -= align;
+       size += align;
+
+       if (!user_access_begin(from, size))
+               return -EFAULT;
+
+       unsafe_get_user(val, (unsigned long __user *) from, err_fault);
+       if (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 < sizeof(unsigned long))
+               val &= aligned_byte_mask(size);
+
+done:
+       user_access_end();
+       return (val == 0);
+err_fault:
+       user_access_end();
+       return -EFAULT;
+}
+EXPORT_SYMBOL(check_zeroed_user);