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

[Arnd Bergmann]

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