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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Wireless]     [Linux Kernel]     [ATH6KL]     [Linux Bluetooth]     [Linux Netdev]     [Kernel Newbies]     [Share Photos]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Device Mapper]

  Powered by Linux