This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH] clone3: validate stack arguments
- From: Oleg Nesterov <oleg at redhat dot com>
- To: Christian Brauner <christian dot brauner at ubuntu dot com>
- Cc: linux-kernel at vger dot kernel dot org, Florian Weimer <fweimer at redhat dot com>, GNU C Library <libc-alpha at sourceware dot org>, Arnd Bergmann <arnd at arndb dot de>, Kees Cook <keescook at chromium dot org>, Jann Horn <jannh at google dot com>, David Howells <dhowells at redhat dot com>, Ingo Molnar <mingo at redhat dot com>, Linus Torvalds <torvalds at linux-foundation dot org>, Peter Zijlstra <peterz at infradead dot org>, linux-api at vger dot kernel dot org, stable at vger dot kernel dot org
- Date: Thu, 31 Oct 2019 17:46:53 +0100
- Subject: Re: [PATCH] clone3: validate stack arguments
- References: <20191031113608.20713-1-christian.brauner@ubuntu.com>
On 10/31, Christian Brauner wrote:
>
> --- a/include/uapi/linux/sched.h
> +++ b/include/uapi/linux/sched.h
> @@ -51,6 +51,10 @@
> * sent when the child exits.
> * @stack: Specify the location of the stack for the
> * child process.
> + * Note, @stack is expected to point to the
> + * lowest address. The stack direction will be
> + * determined by the kernel and set up
> + * appropriately based on @stack_size.
I can't review this patch, I have no idea what does stack_size mean
if !arch/x86.
x86 doesn't use stack_size unless a kthread does kernel_thread(), so
this change is probably fine...
Hmm. Off-topic question, why did 7f192e3cd3 ("fork: add clone3") add
"& ~CSIGNAL" in kernel_thread() ? This looks pointless and confusing
to me...
> +static inline bool clone3_stack_valid(struct kernel_clone_args *kargs)
> +{
> + if (kargs->stack == 0) {
> + if (kargs->stack_size > 0)
> + return false;
> + } else {
> + if (kargs->stack_size == 0)
> + return false;
So to implement clone3_wrapper(void *bottom_of_stack) you need to do
clone3_wrapper(void *bottom_of_stack)
{
struct clone_args args = {
...
// make clone3_stack_valid() happy
.stack = bottom_of_stack - 1,
.stack_size = 1,
};
}
looks a bit strange. OK, I agree, this example is very artificial.
But why do you think clone3() should nack stack_size == 0 ?
> + if (!access_ok((void __user *)kargs->stack, kargs->stack_size))
> + return false;
Why?
Oleg.