Re: [PATCH] clone3: validate stack arguments

[Christian Brauner]

On Fri, Nov 01, 2019 at 01:32:57PM +0100, Oleg Nesterov wrote:
> On 11/01, Christian Brauner wrote:
> >
> > On Thu, Oct 31, 2019 at 05:46:53PM +0100, Oleg Nesterov wrote:
> > > 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.
> >
> > In short: nothing at all if it weren't for ia64 (and maybe parisc).
> > But let me provide some (hopefully useful) context.
> 
> Thanks...
> 
> > (Probably most of
> > that is well-know,
> 
> Certainly not to me ;) Thanks.
> 
> > > > +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 ?
> >
> > In short, consistency.
> 
> And in my opinion this stack_size == 0 check destroys the consistency,
> see below.
> 
> But just in case, let me say that overall I personally like this change.
> 
> > The best thing imho, is to clearly communicate to userspace that stack
> > needs to point to the lowest address and stack_size to the initial range
> > of the stack pointer
> 
> Agreed.
> 
> But the kernel can't verify that "stack" actually points to the lowest
> address and stack_size is actually the stack size. Consider another
> artificial

Sure, but that's the similar to other structs that are passed via a
pointer and come with a size. You could pass:

setxattr(..., ..., value - size, size, ...);

and the kernel would be confused as well.

> 
>     	clone3_wrapper(void *bottom_of_stack, unsigned long offs)
>     	{
>     		struct clone_args args = {
>     			...
>     			// make clone3_stack_valid() happy
>     			.stack = bottom_of_stack - offs,
>     			.stack_size = offs,
>     		};
>     		sys_clone3(args);
>     	}
> 	
> Now,
> 
> 	clone3_wrapper(bottom_of_stack, offs);
> 
> is same thing for _any_ offs except offs == 0 will fail. Why? To me this
> is not consistent, I think the "stack_size == 0" check buys nothing and
> only adds some confusion.

I disagree. It's a very easy contract: pass a stack and a size or
request copy-on-write by passing both as 0.
Sure, you can flaunt that contract but that's true of every other
pointer + size api. The point is: the api we endorse should be simple
and stack + stack_size is very simple.

> 
> Say, stack_size == 1 is "obviously wrong" too, this certainly means that
> "stack" doesn't point to the lowest address (or the child will corrupt the
> memory), but it works.
> 
> OK, I won't insist. Perhaps it can help to detect the case when a user
> forgets to pass the correct stack size.
> 
> > > > +		if (!access_ok((void __user *)kargs->stack, kargs->stack_size))
> > > > +			return false;
> > >
> > > Why?
> >
> > It's nice of us to tell userspace _before_ we have created a thread that
> > it messed up its parameters instead of starting a thread that then
> > immediately crashes.
> 
> Heh. Then why this code doesn't verify that at least stack + stack_size is
> properly mmaped with PROT_READ|WRITE?

access_ok() is uncomplicated.
The other check makes a lot more assumptions. Theare are users that might
want to have a PROT_NONE part of their stack as their own "private"
guard page (Jann just made that point) and there are other corner cases.

Christian