diff mbox series

[RFC,1/8] uaccess: add copy_struct_to_user helper

Message ID 20240902-extensible-structs-check_fields-v1-1-545e93ede2f2@cyphar.com
State Superseded
Headers show
Series extensible syscalls: CHECK_FIELDS to allow for easier feature detection | expand

Commit Message

Aleksa Sarai Sept. 2, 2024, 7:06 a.m. UTC
This is based on copy_struct_from_user(), but there is one additional
case to consider when creating a syscall that returns an
extensible-struct to userspace -- how should data in the struct that
cannot fit into the userspace struct be handled (ksize > usize)?

There are three possibilies:

 1. The interface is like sched_getattr(2), where new information will
    be silently not provided to userspace. This is probably what most
    interfaces will want to do, as it provides the most possible
    backwards-compatibility.

 2. The interface is like lsm_list_modules(2), where you want to return
    an error like -EMSGSIZE if not providing information could result in
    the userspace program making a serious mistake (such as one that
    could lead to a security problem) or if you want to provide some
    flag to userspace so they know that they are missing some
    information.

 3. The interface is like statx(2), where there some kind of a request
    mask that indicates what data userspace would like. One could
    imagine that statx2(2) (using extensible structs) would want to
    return -EMSGSIZE if the user explicitly requested a field that their
    structure is too small to fit, but not return an error if the field
    was not explicitly requested. This is kind of a mix between (1) and
    (2) based on the requested mask.

The copy_struct_to_user() helper includes a an extra argument that is
used to return a boolean flag indicating whether there was a non-zero
byte in the trailing bytes that were not copied to userspace. This can
be used in the following ways to handle all three cases, respectively:

 1. Just pass NULL, as you don't care about this case.

 2. Return an error (say -EMSGSIZE) if the argument was set to true by
    copy_struct_to_user().

 3. If the argument was set to true by copy_struct_to_user(), check if
    there is a flag that implies a field larger than usize.

    This is the only case where callers of copy_struct_to_user() should
    check usize themselves. This will probably require scanning an array
    that specifies what flags were added for each version of the flags
    struct and returning an error if the request mask matches any of the
    flags that were added in versions of the struct that are larger than
    usize.

At the moment we don't have any users of (3), so this patch doesn't
include any helpers to make the necessary scanning easier, but it should
be fairly easy to add some if necessary.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
---
 include/linux/uaccess.h | 98 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

Comments

Arnd Bergmann Sept. 2, 2024, 8:55 a.m. UTC | #1
On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:
> This is based on copy_struct_from_user(), but there is one additional
> case to consider when creating a syscall that returns an
> extensible-struct to userspace -- how should data in the struct that
> cannot fit into the userspace struct be handled (ksize > usize)?
>
> There are three possibilies:
>
>  1. The interface is like sched_getattr(2), where new information will
>     be silently not provided to userspace. This is probably what most
>     interfaces will want to do, as it provides the most possible
>     backwards-compatibility.
>
>  2. The interface is like lsm_list_modules(2), where you want to return
>     an error like -EMSGSIZE if not providing information could result in
>     the userspace program making a serious mistake (such as one that
>     could lead to a security problem) or if you want to provide some
>     flag to userspace so they know that they are missing some
>     information.

I'm not sure if EMSGSIZE is the best choice here, my feeling is that
the kernel should instead try to behave the same way as an older kernel
that did not know about the extra fields:

- if the structure has always been longer than the provided buffer,
  -EMSGSIZE should likely have been returned all along. If older
  kernels just provided a short buffer, changing it now is an ABI
  change that would only affect intentionally broken callers, and
  I think keeping the behavior unchanged is more consistent.

- if an extra flag was added along with the larger buffer size,
  the old kernel would likely have rejected the new flag with -EINVAL,
  so I think returning the same thing for userspace built against
  the old kernel headers is more consistent.


> +static __always_inline __must_check int
> +copy_struct_to_user(void __user *dst, size_t usize, const void *src,
> +		    size_t ksize, bool *ignored_trailing)

This feels like the kind of function that doesn't need to be inline
at all and could be moved to lib/usercopy.c instead. It should clearly
stay in the same place as copy_struct_from_user(), but we could move
that as well.

> +{
> +	size_t size = min(ksize, usize);
> +	size_t rest = max(ksize, usize) - size;
> +
> +	/* Double check if ksize is larger than a known object size. */
> +	if (WARN_ON_ONCE(ksize > __builtin_object_size(src, 1)))
> +		return -E2BIG;

I guess the __builtin_object_size() check is the reason for making
it __always_inline, but that could be done in a trivial inline
wrapper around the extern function.  If ksize is always expected
to be a constant for all callers, the check could even become a
compile-time check instead of a WARN_ON_ONCE() that feels wrong
here: if there is a code path where this can get triggered, there
is clearly a kernel bug, but the only way to find out is to have
a userspace caller that triggers the code path.

Again, the same code is already in copy_struct_from_user(), so
this is not something you are adding but rather something we
may want to change for both.

      Arnd
diff mbox series

Patch

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index d8e4105a2f21..5d0a590ef65d 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -387,6 +387,104 @@  copy_struct_from_user(void *dst, size_t ksize, const void __user *src,
 	return 0;
 }
 
+/**
+ * copy_struct_to_user: copy a struct to userspace
+ * @dst:   Destination address, in userspace. This buffer must be @ksize
+ *         bytes long.
+ * @usize: (Alleged) size of @dst struct.
+ * @src:   Source address, in kernel space.
+ * @ksize: Size of @src struct.
+ * @ignored_trailing: Set to %true if there was a non-zero byte in @src that
+ * userspace cannot see because they are using an smaller struct.
+ *
+ * Copies a struct from kernel space to userspace, 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).
+ *
+ * Some syscalls may wish to make sure that userspace knows about everything in
+ * the struct, and if there is a non-zero value that userspce doesn't know
+ * about, they want to return an error (such as -EMSGSIZE) or have some other
+ * fallback (such as adding a "you're missing some information" flag). If
+ * @ignored_trailing is non-%NULL, it will be set to %true if there was a
+ * non-zero byte that could not be copied to userspace (ie. was past @usize).
+ *
+ * While unconditionally returning an error in this case is the simplest
+ * solution, for maximum backward compatibility you should try to only return
+ * -EMSGSIZE if the user explicitly requested the data that couldn't be copied.
+ * Note that structure sizes can change due to header changes and simple
+ * recompilations without code changes(!), so if you care about
+ * @ignored_trailing you probably want to make sure that any new field data is
+ * associated with a flag. Otherwise you might assume that a program knows
+ * about data it does not.
+ *
+ * @ksize is just sizeof(*src), and @usize should've been passed by userspace.
+ * The recommended usage is something like the following:
+ *
+ *   SYSCALL_DEFINE2(foobar, struct foo __user *, uarg, size_t, usize)
+ *   {
+ *      int err;
+ *      bool ignored_trailing;
+ *      struct foo karg = {};
+ *
+ *      if (usize > PAGE_SIZE)
+ *		return -E2BIG;
+ *      if (usize < FOO_SIZE_VER0)
+ *		return -EINVAL;
+ *
+ *      // ... modify karg somehow ...
+ *
+ *      err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg),
+ *				  &ignored_trailing);
+ *      if (err)
+ *		return err;
+ *      if (ignored_trailing)
+ *		return -EMSGSIZE:
+ *
+ *      // ...
+ *   }
+ *
+ * There are three cases to consider:
+ *  * If @usize == @ksize, then it's copied verbatim.
+ *  * If @usize < @ksize, then the kernel is trying to pass userspace a newer
+ *    struct than it supports. Thus we only copy the interoperable portions
+ *    (@usize) and ignore the rest (but @ignored_trailing is set to %true if
+ *    any of the trailing (@ksize - @usize) bytes are non-zero).
+ *  * If @usize > @ksize, then the kernel is trying to pass userspace an older
+ *    struct than userspace supports. In order to make sure the
+ *    unknown-to-the-kernel fields don't contain garbage values, we zero the
+ *    trailing (@usize - @ksize) bytes.
+ *
+ * Returns (in all cases, some data may have been copied):
+ *  * -EFAULT: access to userspace failed.
+ */
+static __always_inline __must_check int
+copy_struct_to_user(void __user *dst, size_t usize, const void *src,
+		    size_t ksize, bool *ignored_trailing)
+{
+	size_t size = min(ksize, usize);
+	size_t rest = max(ksize, usize) - size;
+
+	/* Double check if ksize is larger than a known object size. */
+	if (WARN_ON_ONCE(ksize > __builtin_object_size(src, 1)))
+		return -E2BIG;
+
+	/* Deal with trailing bytes. */
+	if (usize > ksize) {
+		int ret = clear_user(dst + size, rest);
+		if (ret)
+			return ret;
+	}
+	if (ignored_trailing)
+		*ignored_trailing = ksize < usize &&
+			memchr_inv(src + size, 0, rest) != NULL;
+	/* Copy the interoperable parts of the struct. */
+	if (copy_to_user(dst, src, size))
+		return -EFAULT;
+	return 0;
+}
+
 bool copy_from_kernel_nofault_allowed(const void *unsafe_src, size_t size);
 
 long copy_from_kernel_nofault(void *dst, const void *src, size_t size);