[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[James Morse]

Hi all,

This version of Sahara's arch_within_stack_frames() series replaces the
open-coded stack walker with a call to arm64's existing walker.

Patch 2 can be tested independently with this change[0].

lkdtm's use of unallocated stack regions is a separate problem, patch 3
tries to address this.

Sahara, it would be good to get your review of this!
I'm afraid I omitted your patch-3 as it stopped the lkdtm test from working,
I suspect its not tricking the compiler, but I haven't investigated.


Thanks,

James

[0] Change to lkdtm to generate accesses that overlap stack frames.
--------------%<--------------
diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
index 1dd611423d8b..fcbba3a14387 100644
--- a/drivers/misc/lkdtm_usercopy.c
+++ b/drivers/misc/lkdtm_usercopy.c
@@ -57,7 +57,8 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 
        /* This is a pointer to outside our current stack frame. */
        if (bad_frame) {
-               bad_stack = do_usercopy_stack_callee((uintptr_t)&bad_stack);
+               bad_stack = __builtin_frame_address(0);
+               bad_stack -= sizeof(good_stack)/2;
        } else {
                /* Put start address just inside stack. */
                bad_stack = task_stack_page(current) + THREAD_SIZE;
--------------%<--------------


James Morse (2):
  arm64: Add arch_within_stack_frames() for hardened usercopy
  arm64/uaccess: Add hardened usercopy check for bad stack accesses

Sahara (1):
  usercopy: create enum stack_type

 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h |  7 ++++-
 arch/arm64/include/asm/uaccess.h     | 20 +++++++++++++
 arch/arm64/kernel/stacktrace.c       | 54 ++++++++++++++++++++++++++++++++++--
 arch/x86/include/asm/thread_info.h   | 19 +++++++------
 include/linux/thread_info.h          | 13 +++++++--
 mm/usercopy.c                        |  8 +-----
 7 files changed, 99 insertions(+), 23 deletions(-)

-- 
2.10.1

[PATCH v4 1/3] usercopy: create enum stack_type

[James Morse]

From: Sahara <keun-o.park@darkmatter.ae>

This patch creates enum stack_type which is only used in usercopy.c
for now. This enum type can be used for x86 and other architecture's
thread_info.h, which may have arch_within_stack_frames().

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Signed-off-by: James Morse <james.morse@arm.com>
Reviewed-by: Kees Cook <keescook@chromium.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
 arch/x86/include/asm/thread_info.h | 19 ++++++++++---------
 include/linux/thread_info.h        | 13 ++++++++++---
 mm/usercopy.c                      |  8 +-------
 3 files changed, 21 insertions(+), 19 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index ad6f5eb07a95..7af4b8b23b8b 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -168,13 +168,13 @@ static inline unsigned long current_stack_pointer(void)
  * entirely contained by a single stack frame.
  *
  * Returns:
- *		 1 if within a frame
- *		-1 if placed across a frame boundary (or outside stack)
- *		 0 unable to determine (no frame pointers, etc)
+ *	GOOD_FRAME	if within a frame
+ *	BAD_STACK	if placed across a frame boundary (or outside stack)
+ *	NOT_STACK	unable to determine (no frame pointers, etc)
  */
-static inline int arch_within_stack_frames(const void * const stack,
-					   const void * const stackend,
-					   const void *obj, unsigned long len)
+static inline enum stack_type arch_within_stack_frames(const void * const stack,
+					const void * const stackend,
+					const void *obj, unsigned long len)
 {
 #if defined(CONFIG_FRAME_POINTER)
 	const void *frame = NULL;
@@ -197,13 +197,14 @@ static inline int arch_within_stack_frames(const void * const stack,
 		 * the copy as invalid.
 		 */
 		if (obj + len <= frame)
-			return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
+			return obj >= oldframe + 2 * sizeof(void *) ?
+				GOOD_FRAME : BAD_STACK;
 		oldframe = frame;
 		frame = *(const void * const *)frame;
 	}
-	return -1;
+	return BAD_STACK;
 #else
-	return 0;
+	return NOT_STACK;
 #endif
 }
 
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 58373875e8ee..a38b3be347df 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -22,6 +22,13 @@
 #endif
 
 #include <linux/bitops.h>
+
+enum stack_type {
+	BAD_STACK = -1,
+	NOT_STACK = 0,
+	GOOD_FRAME,
+	GOOD_STACK,
+};
 #include <asm/thread_info.h>
 
 #ifdef __KERNEL__
@@ -77,9 +84,9 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 #define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
 
 #ifndef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
-static inline int arch_within_stack_frames(const void * const stack,
-					   const void * const stackend,
-					   const void *obj, unsigned long len)
+static inline enum stack_type arch_within_stack_frames(const void * const stack,
+					const void * const stackend,
+					const void *obj, unsigned long len)
 {
 	return 0;
 }
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 3c8da0af9695..3531ae735d87 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -16,15 +16,9 @@
 
 #include <linux/mm.h>
 #include <linux/slab.h>
+#include <linux/thread_info.h>
 #include <asm/sections.h>
 
-enum {
-	BAD_STACK = -1,
-	NOT_STACK = 0,
-	GOOD_FRAME,
-	GOOD_STACK,
-};
-
 /*
  * Checks if a given pointer and length is contained by the current
  * stack frame (if possible).
-- 
2.10.1

[PATCH v4 1/3] usercopy: create enum stack_type

[Kees Cook]

On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> This patch creates enum stack_type which is only used in usercopy.c
> for now. This enum type can be used for x86 and other architecture's
> thread_info.h, which may have arch_within_stack_frames().
>
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Acked-by: Kees Cook <keescook@chromium.org>

I've adjusted this slightly and added it to my usercopy tree for -next.

-Kees

> ---
>  arch/x86/include/asm/thread_info.h | 19 ++++++++++---------
>  include/linux/thread_info.h        | 13 ++++++++++---
>  mm/usercopy.c                      |  8 +-------
>  3 files changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index ad6f5eb07a95..7af4b8b23b8b 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -168,13 +168,13 @@ static inline unsigned long current_stack_pointer(void)
>   * entirely contained by a single stack frame.
>   *
>   * Returns:
> - *              1 if within a frame
> - *             -1 if placed across a frame boundary (or outside stack)
> - *              0 unable to determine (no frame pointers, etc)
> + *     GOOD_FRAME      if within a frame
> + *     BAD_STACK       if placed across a frame boundary (or outside stack)
> + *     NOT_STACK       unable to determine (no frame pointers, etc)
>   */
> -static inline int arch_within_stack_frames(const void * const stack,
> -                                          const void * const stackend,
> -                                          const void *obj, unsigned long len)
> +static inline enum stack_type arch_within_stack_frames(const void * const stack,
> +                                       const void * const stackend,
> +                                       const void *obj, unsigned long len)
>  {
>  #if defined(CONFIG_FRAME_POINTER)
>         const void *frame = NULL;
> @@ -197,13 +197,14 @@ static inline int arch_within_stack_frames(const void * const stack,
>                  * the copy as invalid.
>                  */
>                 if (obj + len <= frame)
> -                       return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
> +                       return obj >= oldframe + 2 * sizeof(void *) ?
> +                               GOOD_FRAME : BAD_STACK;
>                 oldframe = frame;
>                 frame = *(const void * const *)frame;
>         }
> -       return -1;
> +       return BAD_STACK;
>  #else
> -       return 0;
> +       return NOT_STACK;
>  #endif
>  }
>
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 58373875e8ee..a38b3be347df 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -22,6 +22,13 @@
>  #endif
>
>  #include <linux/bitops.h>
> +
> +enum stack_type {
> +       BAD_STACK = -1,
> +       NOT_STACK = 0,
> +       GOOD_FRAME,
> +       GOOD_STACK,
> +};
>  #include <asm/thread_info.h>
>
>  #ifdef __KERNEL__
> @@ -77,9 +84,9 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
>  #define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
>
>  #ifndef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
> -static inline int arch_within_stack_frames(const void * const stack,
> -                                          const void * const stackend,
> -                                          const void *obj, unsigned long len)
> +static inline enum stack_type arch_within_stack_frames(const void * const stack,
> +                                       const void * const stackend,
> +                                       const void *obj, unsigned long len)
>  {
>         return 0;
>  }
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 3c8da0af9695..3531ae735d87 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -16,15 +16,9 @@
>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/thread_info.h>
>  #include <asm/sections.h>
>
> -enum {
> -       BAD_STACK = -1,
> -       NOT_STACK = 0,
> -       GOOD_FRAME,
> -       GOOD_STACK,
> -};
> -
>  /*
>   * Checks if a given pointer and length is contained by the current
>   * stack frame (if possible).
> --
> 2.10.1
>



-- 
Kees Cook
Pixel Security

[PATCH v4 2/3] arm64: Add arch_within_stack_frames() for hardened usercopy

[James Morse]

Hardened usercopy tests that an object being copied to/from userspace
doesn't overlap multiple stack frames.

Add arch_within_stack_frames() to do this using arm64's stackwalker. The
callback looks for 'fp' appearing with the range occupied by the object.

(This isn't enough to trip the lkdtm tests on arm64)

CC: Sahara <keun-o.park@darkmatter.ae>
Based-on-a-patch-from: Sahara <keun-o.park@darkmatter.ae>
Signed-off-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h |  7 ++++-
 arch/arm64/kernel/stacktrace.c       | 54 ++++++++++++++++++++++++++++++++++--
 3 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 111742126897..378caa9c0563 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -67,6 +67,7 @@ config ARM64
 	select HAVE_ARCH_TRACEHOOK
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
 	select HAVE_ARM_SMCCC
+	select HAVE_ARCH_WITHIN_STACK_FRAMES
 	select HAVE_EBPF_JIT
 	select HAVE_C_RECORDMCOUNT
 	select HAVE_CC_STACKPROTECTOR
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 46c3b93cf865..3540c46027fc 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -68,7 +68,12 @@ struct thread_info {
 #define thread_saved_fp(tsk)	\
 	((unsigned long)(tsk->thread.cpu_context.fp))
 
-#endif
+
+extern enum stack_type arch_within_stack_frames(const void * const stack,
+						const void * const stackend,
+						const void *obj,
+						unsigned long len);
+#endif /* !__ASSEMBLY__ */
 
 /*
  * thread information flags:
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 8a552a33c6ef..5591f325729e 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -25,6 +25,12 @@
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>
 
+#define FAKE_FRAME(frame, my_func) do {			\
+	frame.fp = (unsigned long)__builtin_frame_address(0);	\
+	frame.sp = current_stack_pointer;		\
+	frame.pc = (unsigned long)my_func;		\
+} while (0)
+
 /*
  * AArch64 PCS assigns the frame pointer to x29.
  *
@@ -194,9 +200,7 @@ void save_stack_trace_tsk(struct task_struct *tsk, struct stack_trace *trace)
 		frame.pc = thread_saved_pc(tsk);
 	} else {
 		data.no_sched_functions = 0;
-		frame.fp = (unsigned long)__builtin_frame_address(0);
-		frame.sp = current_stack_pointer;
-		frame.pc = (unsigned long)save_stack_trace_tsk;
+		FAKE_FRAME(frame, save_stack_trace_tsk);
 	}
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 	frame.graph = tsk->curr_ret_stack;
@@ -215,3 +219,47 @@ void save_stack_trace(struct stack_trace *trace)
 }
 EXPORT_SYMBOL_GPL(save_stack_trace);
 #endif
+
+struct check_frame_arg {
+	unsigned long obj_start;
+	unsigned long obj_end;
+	int err;
+};
+
+static int check_frame(struct stackframe *frame, void *d)
+{
+	struct check_frame_arg *arg = d;
+
+	/* object overlaps multiple frames */
+	if (arg->obj_start < (frame->fp + 0x10) && frame->fp < arg->obj_end) {
+		arg->err = BAD_STACK;
+		return 1;
+	}
+
+	/* walked past the object */
+	if (arg->obj_end < frame->fp)
+		return 1;
+
+	return 0;
+}
+
+/* Check obj doesn't overlap a stack frame record */
+enum stack_type arch_within_stack_frames(const void *stack,
+					 const void *stack_end,
+					 const void *obj, unsigned long obj_len)
+{
+	struct stackframe frame;
+	struct check_frame_arg arg;
+
+	if (!IS_ENABLED(CONFIG_FRAME_POINTER))
+		return NOT_STACK;
+
+	arg.err = GOOD_FRAME;
+	arg.obj_start = (unsigned long)obj;
+	arg.obj_end = arg.obj_start + obj_len;
+
+	FAKE_FRAME(frame, arch_within_stack_frames);
+	walk_stackframe(current, &frame, check_frame, &arg);
+
+	return arg.err;
+}
-- 
2.10.1

[PATCH v4 2/3] arm64: Add arch_within_stack_frames() for hardened usercopy

[Kees Cook]

On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
> Hardened usercopy tests that an object being copied to/from userspace
> doesn't overlap multiple stack frames.
>
> Add arch_within_stack_frames() to do this using arm64's stackwalker. The
> callback looks for 'fp' appearing with the range occupied by the object.
>
> (This isn't enough to trip the lkdtm tests on arm64)
>
> CC: Sahara <keun-o.park@darkmatter.ae>
> Based-on-a-patch-from: Sahara <keun-o.park@darkmatter.ae>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
>  arch/arm64/Kconfig                   |  1 +
>  arch/arm64/include/asm/thread_info.h |  7 ++++-
>  arch/arm64/kernel/stacktrace.c       | 54 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 58 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 111742126897..378caa9c0563 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -67,6 +67,7 @@ config ARM64
>         select HAVE_ARCH_TRACEHOOK
>         select HAVE_ARCH_TRANSPARENT_HUGEPAGE
>         select HAVE_ARM_SMCCC
> +       select HAVE_ARCH_WITHIN_STACK_FRAMES
>         select HAVE_EBPF_JIT
>         select HAVE_C_RECORDMCOUNT
>         select HAVE_CC_STACKPROTECTOR
> diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
> index 46c3b93cf865..3540c46027fc 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -68,7 +68,12 @@ struct thread_info {
>  #define thread_saved_fp(tsk)   \
>         ((unsigned long)(tsk->thread.cpu_context.fp))
>
> -#endif
> +
> +extern enum stack_type arch_within_stack_frames(const void * const stack,
> +                                               const void * const stackend,
> +                                               const void *obj,
> +                                               unsigned long len);

The caller of arch_within_stack_frames expects this to be inlined...
could that be changed and then move the special stack check from the
third patch into check_stack_object() directly?

Regardless, I'm fine with reusing the existing walker. I just want to
avoid special cases in the uaccess code (so we can consolidate it in
the future).

-Kees

-- 
Kees Cook
Pixel Security

[PATCH v4 3/3] arm64/uaccess: Add hardened usercopy check for bad stack accesses

[James Morse]

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@darkmatter.ae>
Signed-off-by: James Morse <james.morse@arm.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);
+}
+
 extern unsigned long __must_check __arch_copy_from_user(void *to, const void __user *from, unsigned long n);
 extern unsigned long __must_check __arch_copy_to_user(void __user *to, const void *from, unsigned long n);
 extern unsigned long __must_check __copy_in_user(void __user *to, const void __user *from, unsigned long n);
@@ -364,6 +380,7 @@ extern unsigned long __must_check __clear_user(void __user *addr, unsigned long
 static inline unsigned long __must_check __copy_from_user(void *to, const void __user *from, unsigned long n)
 {
 	kasan_check_write(to, n);
+	check_obj_in_unused_stack(to, n);
 	check_object_size(to, n, false);
 	return __arch_copy_from_user(to, from, n);
 }
@@ -371,6 +388,7 @@ static inline unsigned long __must_check __copy_from_user(void *to, const void _
 static inline unsigned long __must_check __copy_to_user(void __user *to, const void *from, unsigned long n)
 {
 	kasan_check_read(from, n);
+	check_obj_in_unused_stack(from, n);
 	check_object_size(from, n, true);
 	return __arch_copy_to_user(to, from, n);
 }
@@ -381,6 +399,7 @@ static inline unsigned long __must_check copy_from_user(void *to, const void __u
 	kasan_check_write(to, n);
 
 	if (access_ok(VERIFY_READ, from, n)) {
+		check_obj_in_unused_stack(to, n);
 		check_object_size(to, n, false);
 		res = __arch_copy_from_user(to, from, n);
 	}
@@ -394,6 +413,7 @@ static inline unsigned long __must_check copy_to_user(void __user *to, const voi
 	kasan_check_read(from, n);
 
 	if (access_ok(VERIFY_WRITE, to, n)) {
+		check_obj_in_unused_stack(from, n);
 		check_object_size(from, n, true);
 		n = __arch_copy_to_user(to, from, n);
 	}
-- 
2.10.1

[PATCH v4 3/3] arm64/uaccess: Add hardened usercopy check for bad stack accesses

[Kees Cook]

On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.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@darkmatter.ae>
> Signed-off-by: James Morse <james.morse@arm.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

[kernel-hardening] [PATCH v4 3/3] arm64/uaccess: Add hardened usercopy check for bad stack accesses

[Keun-O Park]

Hello James,

> 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 looks to me that this function is just doing the similar check that
check_stack_object() may do.
Probably I guess you had a problem in checking the correct fp of
caller's function while you tried to use walk_stackframe().
Anyway, your patch works fine for me.
But, I can not find any reason to create check_obj_in_unused_stack().
Instead of creating check_obj_in_unused_stack(), how about handing
over current_stack_pointer to check_stack_object();
Even though Akashi's case happens, since we hand over correct stack
pointer, it can check if the obj is within the correct stack boundary.

diff --git a/arch/arm64/include/asm/uaccess.h b/arch/arm64/include/asm/uaccess.h
index d349484..0c21a0d 100644
--- a/arch/arm64/include/asm/uaccess.h
+++ b/arch/arm64/include/asm/uaccess.h
@@ -356,22 +356,6 @@ 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);
-}
-
 extern unsigned long __must_check __arch_copy_from_user(void *to,
const void __user *from, unsigned long n);
 extern unsigned long __must_check __arch_copy_to_user(void __user
*to, const void *from, unsigned long n);
 extern unsigned long __must_check __copy_in_user(void __user *to,
const void __user *from, unsigned long n);
@@ -380,16 +364,14 @@ extern unsigned long __must_check
__clear_user(void __user *addr, unsigned long
 static inline unsigned long __must_check __copy_from_user(void *to,
const void __user *from, unsigned long n)
 {
        kasan_check_write(to, n);
-       check_obj_in_unused_stack(to, n);
-       check_object_size(to, n, false);
+       check_object_size(to, n, current_stack_pointer, false);
        return __arch_copy_from_user(to, from, n);
 }

 static inline unsigned long __must_check __copy_to_user(void __user
*to, const void *from, unsigned long n)
 {
        kasan_check_read(from, n);
-       check_obj_in_unused_stack(from, n);
-       check_object_size(from, n, true);
+       check_object_size(from, n, current_stack_pointer, true);
        return __arch_copy_to_user(to, from, n);
 }

@@ -399,8 +381,7 @@ static inline unsigned long __must_check
copy_from_user(void *to, const void __u
        kasan_check_write(to, n);

        if (access_ok(VERIFY_READ, from, n)) {
-               check_obj_in_unused_stack(to, n);
-               check_object_size(to, n, false);
+               check_object_size(to, n, current_stack_pointer, false);
                res = __arch_copy_from_user(to, from, n);
        }
        if (unlikely(res))
@@ -413,8 +394,7 @@ static inline unsigned long __must_check
copy_to_user(void __user *to, const voi
        kasan_check_read(from, n);

        if (access_ok(VERIFY_WRITE, to, n)) {
-               check_obj_in_unused_stack(from, n);
-               check_object_size(from, n, true);
+               check_object_size(from, n, current_stack_pointer, true);
                n = __arch_copy_to_user(to, from, n);
        }
        return n;
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index a38b3be..5c676bc 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -94,17 +94,17 @@ static inline enum stack_type
arch_within_stack_frames(const void * const stack,

 #ifdef CONFIG_HARDENED_USERCOPY
 extern void __check_object_size(const void *ptr, unsigned long n,
-                                       bool to_user);
+                               unsigned long sp, bool to_user);

 static __always_inline void check_object_size(const void *ptr, unsigned long n,
-                                             bool to_user)
+                                             unsigned long sp, bool to_user)
 {
        if (!__builtin_constant_p(n))
-               __check_object_size(ptr, n, to_user);
+               __check_object_size(ptr, n, sp, to_user);
 }
 #else
 static inline void check_object_size(const void *ptr, unsigned long n,
-                                    bool to_user)
+                                    unsigned long sp, bool to_user)
 { }
 #endif /* CONFIG_HARDENED_USERCOPY */

diff --git a/lib/strncpy_from_user.c b/lib/strncpy_from_user.c
index 7e35fc4..4118da4 100644
--- a/lib/strncpy_from_user.c
+++ b/lib/strncpy_from_user.c
@@ -112,7 +112,7 @@ long strncpy_from_user(char *dst, const char
__user *src, long count)
                long retval;

                kasan_check_write(dst, count);
-               check_object_size(dst, count, false);
+               check_object_size(dst, count, current_stack_pointer, false);
                user_access_begin();
                retval = do_strncpy_from_user(dst, src, count, max);
                user_access_end();
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 3531ae7..3739022 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -29,7 +29,8 @@
  *     GOOD_STACK: fully on the stack (when can't do frame-checking)
  *     BAD_STACK: error condition (invalid stack position or bad stack frame)
  */
-static noinline int check_stack_object(const void *obj, unsigned long len)
+static noinline int check_stack_object(const void *obj, unsigned long len,
+                                      unsigned long usercopy_sp)
 {
        const void * const stack = task_stack_page(current);
        const void * const stackend = stack + THREAD_SIZE;
@@ -44,7 +45,7 @@ static noinline int check_stack_object(const void
*obj, unsigned long len)
         * the check above means@least one end is within the stack,
         * so if this check fails, the other end is outside the stack).
         */
-       if (obj < stack || stackend < obj + len)
+       if (obj < usercopy_sp || stackend < obj + len)
                return BAD_STACK;

        /* Check if object is safely within a valid frame. */
@@ -227,7 +229,8 @@ static inline const char *check_heap_object(const
void *ptr, unsigned long n,
  * - known-safe heap or stack object
  * - not in kernel text
  */
-void __check_object_size(const void *ptr, unsigned long n, bool to_user)
+void __check_object_size(const void *ptr, unsigned long n,
+                        unsigned long sp, bool to_user)
 {
        const char *err;

@@ -246,7 +249,7 @@ void __check_object_size(const void *ptr, unsigned
long n, bool to_user)
                goto report;

        /* Check for bad stack object. */
-       switch (check_stack_object(ptr, n)) {
+       switch (check_stack_object(ptr, n, sp)) {
        case NOT_STACK:
                /* Object is not touching the current process stack. */
                break;


Thanks so much for your patches.

BR
Sahara

[kernel-hardening] [PATCH v4 3/3] arm64/uaccess: Add hardened usercopy check for bad stack accesses

[James Morse]

Hi,

On 17/02/17 18:09, Keun-O Park wrote:
>> 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 {                                                                       \

>> +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 looks to me that this function is just doing the similar check that
> check_stack_object() may do.
> Probably I guess you had a problem in checking the correct fp of
> caller's function while you tried to use walk_stackframe().

The value needs to be taken in a hopefully-inlined function if you want to catch
LKDTMs 'just off the stack' writes. Doing it like this saved meddling with the
generic code.


> Instead of creating check_obj_in_unused_stack(), how about handing
> over current_stack_pointer to check_stack_object();

Sure, do you want to give this a go? (I have my hands full at the moment)

It might not be the right check on all architectures, x86 redzone comes to mind,
but it doesn't look like they use this in the kernel.


There is also Al Viro's uaccess unification work that may affect whether this is
worth doing now:
https://lkml.org/lkml/2015/12/3/494



Thanks,

James

[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[Kees Cook]

On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
> Hi all,
>
> This version of Sahara's arch_within_stack_frames() series replaces the
> open-coded stack walker with a call to arm64's existing walker.
>
> Patch 2 can be tested independently with this change[0].
>
> lkdtm's use of unallocated stack regions is a separate problem, patch 3
> tries to address this.
>
> Sahara, it would be good to get your review of this!
> I'm afraid I omitted your patch-3 as it stopped the lkdtm test from working,
> I suspect its not tricking the compiler, but I haven't investigated.
>
>
> Thanks,
>
> James
>
> [0] Change to lkdtm to generate accesses that overlap stack frames.
> --------------%<--------------
> diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
> index 1dd611423d8b..fcbba3a14387 100644
> --- a/drivers/misc/lkdtm_usercopy.c
> +++ b/drivers/misc/lkdtm_usercopy.c
> @@ -57,7 +57,8 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
>
>         /* This is a pointer to outside our current stack frame. */
>         if (bad_frame) {
> -               bad_stack = do_usercopy_stack_callee((uintptr_t)&bad_stack);
> +               bad_stack = __builtin_frame_address(0);
> +               bad_stack -= sizeof(good_stack)/2;

Ah, sneaky, yeah, that'll work nicely.

(Though it should likely get wrapped in a CONFIG_STACK_GROWSUP/DOWN test...)

-Kees

-- 
Kees Cook
Pixel Security

[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[Kees Cook]

On Thu, Feb 16, 2017 at 4:54 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
>> Hi all,
>>
>> This version of Sahara's arch_within_stack_frames() series replaces the
>> open-coded stack walker with a call to arm64's existing walker.
>>
>> Patch 2 can be tested independently with this change[0].
>>
>> lkdtm's use of unallocated stack regions is a separate problem, patch 3
>> tries to address this.
>>
>> Sahara, it would be good to get your review of this!
>> I'm afraid I omitted your patch-3 as it stopped the lkdtm test from working,
>> I suspect its not tricking the compiler, but I haven't investigated.
>>
>>
>> Thanks,
>>
>> James
>>
>> [0] Change to lkdtm to generate accesses that overlap stack frames.
>> --------------%<--------------
>> diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
>> index 1dd611423d8b..fcbba3a14387 100644
>> --- a/drivers/misc/lkdtm_usercopy.c
>> +++ b/drivers/misc/lkdtm_usercopy.c
>> @@ -57,7 +57,8 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
>>
>>         /* This is a pointer to outside our current stack frame. */
>>         if (bad_frame) {
>> -               bad_stack = do_usercopy_stack_callee((uintptr_t)&bad_stack);
>> +               bad_stack = __builtin_frame_address(0);
>> +               bad_stack -= sizeof(good_stack)/2;
>
> Ah, sneaky, yeah, that'll work nicely.
>
> (Though it should likely get wrapped in a CONFIG_STACK_GROWSUP/DOWN test...)

Is this still in progress? Seemed like it was very close?

-Kees

-- 
Kees Cook
Pixel Security

[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[James Morse]

Hi Kees,

On 28/03/17 23:34, Kees Cook wrote:
> On Thu, Feb 16, 2017 at 4:54 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
>>> This version of Sahara's arch_within_stack_frames() series replaces the
>>> open-coded stack walker with a call to arm64's existing walker.

> Is this still in progress? Seemed like it was very close?

Ah, sorry, I lost track of this when it jumped between mail folders... Sahara
had comments on the last patch.

How does all this fit with Al Viro's uaccess unification tree?:
https://lkml.org/lkml/2017/3/29/61


Thanks,

James

[PATCH v4 0/3] arm64: usercopy: Implement stack frame object validation

[Kees Cook]

On Thu, Mar 30, 2017 at 1:30 AM, James Morse <james.morse@arm.com> wrote:
> Hi Kees,
>
> On 28/03/17 23:34, Kees Cook wrote:
>> On Thu, Feb 16, 2017 at 4:54 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Thu, Feb 16, 2017 at 10:29 AM, James Morse <james.morse@arm.com> wrote:
>>>> This version of Sahara's arch_within_stack_frames() series replaces the
>>>> open-coded stack walker with a call to arm64's existing walker.
>
>> Is this still in progress? Seemed like it was very close?
>
> Ah, sorry, I lost track of this when it jumped between mail folders... Sahara
> had comments on the last patch.
>
> How does all this fit with Al Viro's uaccess unification tree?:
> https://lkml.org/lkml/2017/3/29/61

It's orthogonal, though it results in bringing the hardened usercopy
to more architectures... (but the stack walker is still needed on a
per-arch basis).

-Kees

-- 
Kees Cook
Pixel Security