On 2024-09-02, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> 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:
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.
>
>
> > +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.
> > +{
> > + 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/>
Attachment:
signature.asc
Description: PGP signature
