linux-kernel - Re: [PATCH RFC 1/8] uaccess: add copy_struct_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240902.090015-soviet.bowling.selfish.chin-kvAI3lnuyqH@cyphar.com>
Date: Tue, 3 Sep 2024 02:02:39 +1000
From: Aleksa Sarai <cyphar@...har.com>
To: Arnd Bergmann <arnd@...db.de>
Cc: Ingo Molnar <mingo@...hat.com>, Peter Zijlstra <peterz@...radead.org>, 
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt <rostedt@...dmis.org>, 
	Benjamin Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Valentin Schneider <vschneid@...hat.com>, Alexander Viro <viro@...iv.linux.org.uk>, 
	Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>, shuah <shuah@...nel.org>, 
	Kees Cook <kees@...nel.org>, Florian Weimer <fweimer@...hat.com>, 
	Mark Rutland <mark.rutland@....com>, linux-kernel@...r.kernel.org, linux-api@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, Linux-Arch <linux-arch@...r.kernel.org>, 
	linux-kselftest@...r.kernel.org
Subject: Re: [PATCH RFC 1/8] uaccess: add copy_struct_to_user helper

On 2024-09-02, Arnd Bergmann <arnd@...db.de> 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/>

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)