Re: [PATCH RFC 1/8] uaccess: add copy_struct_to_user helper

On 2024-09-02, Arnd Bergmann [off-list ref] wrote:

On Mon, Sep 2, 2024, at 07:06, Aleksa Sarai wrote:

quoted

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:

I agree this API is not ideal for syscalls because it can lead to
backward-compatibility issues, but that is how lsm_list_modules(2)
works. I suspect most syscalls will go with designs (1) or (3).

- 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.

quoted

+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.

IIRC Kees suggested copy_struct_from_user() be inline when I first
included it, though I would have to dig through the old threads to find
the reasoning. __builtin_object_size() was added some time after it was
merged so that wasn't the original reason.

quoted

+{
+	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

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