Openwall GNU/*/Linux - a small security-enhanced Linux distro for servers
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Thu, 16 Feb 2017 16:44:04 -0800
From: Kees Cook <keescook@...omium.org>
To: James Morse <james.morse@....com>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, 
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>, Will Deacon <will.deacon@....com>, 
	Catalin Marinas <catalin.marinas@....com>, Mark Rutland <mark.rutland@....com>, 
	Pratyush Anand <panand@...hat.com>, keun-o.park@...kmatter.ae
Subject: Re: [PATCH v4 3/3] arm64/uaccess: Add hardened usercopy check for bad
 stack accesses

On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@....com> wrote:
> lkdtm tests copy_{to,from}_user() by trying to copy an address range
> on the stack that isn't yet part of a stack frame.
>
> By the time the stack walker is invoked to check that the object being
> copied doesn't overlap stack frame, the invalid range is part of a valid
> stack frame. Discarding a constant number of frames is fragile as different
> compiler versions may make different inline choices.
>
> Instead, add a check that the object isn't between the current stack
> pointer and the end of the stack. Add this early enough that it should
> be inlined into the caller.
>
> CC: Sahara <keun-o.park@...kmatter.ae>
> Signed-off-by: James Morse <james.morse@....com>
> ---
>  arch/arm64/include/asm/uaccess.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
> index 46da3ea638bb..d3494840a61c 100644
> --- a/arch/arm64/include/asm/uaccess.h
> +++ b/arch/arm64/include/asm/uaccess.h
> @@ -356,6 +356,22 @@ do {                                                                       \
>                 -EFAULT;                                                \
>  })
>
> +static inline void check_obj_in_unused_stack(const void *obj, unsigned long len)
> +{
> +       unsigned long stack = (unsigned long)task_stack_page(current);
> +
> +       if (__builtin_constant_p(len) || !IS_ENABLED(CONFIG_HARDENED_USERCOPY) || !len)
> +               return;
> +
> +       /*
> +        * If current_stack_pointer is on the task stack, obj must not lie
> +        * between current_stack_pointer and the last stack address.
> +        */
> +       if ((current_stack_pointer & ~(THREAD_SIZE-1)) == stack)
> +               BUG_ON(stack <= (unsigned long)obj &&
> +                      (unsigned long)obj < current_stack_pointer);
> +}

It seems like this would be a valid test on all architecture, yes? I
wonder if this could be reworked so we don't have a special case for
ARM here...

-Kees

-- 
Kees Cook
Pixel Security

Powered by blists - more mailing lists

Your e-mail address:

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.