Re: [PATCH] clone3: validate stack arguments

On 11/01, Christian Brauner wrote:

On Thu, Oct 31, 2019 at 05:46:53PM +0100, Oleg Nesterov wrote:

quoted

On 10/31, Christian Brauner wrote:

quoted

--- 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.

quoted

quoted

+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

    	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.

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.

quoted

quoted

+		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?

Oleg.