[kernel-hardening] [PATCH v2 1/3] usercopy: create usercopy.h and move enum stack_type

[kernel-hardening] [PATCH v2 1/3] usercopy: create usercopy.h and move enum stack_type

[kpark3469]

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

This patch creates linux/usercopy.h and moves the enum which is
only used in usercopy.c so as to make it usable to x86 and other
architecture's thread_info.h, which may have arch_within_stack_frames().

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Suggested-by: James Morse <james.morse@arm.com>
---
 arch/x86/include/asm/thread_info.h | 20 +++++++++++---------
 include/linux/usercopy.h           | 11 +++++++++++
 mm/usercopy.c                      |  8 +-------
 3 files changed, 23 insertions(+), 16 deletions(-)
 create mode 100644 include/linux/usercopy.h

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index ad6f5eb0..c991126 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -51,6 +51,7 @@
 struct task_struct;
 #include <asm/cpufeature.h>
 #include <linux/atomic.h>
+#include <linux/usercopy.h>
 
 struct thread_info {
 	unsigned long		flags;		/* low level flags */
@@ -168,13 +169,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 +198,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/usercopy.h b/include/linux/usercopy.h
new file mode 100644
index 0000000..974859e
--- /dev/null
+++ b/include/linux/usercopy.h
@@ -0,0 +1,11 @@
+#ifndef __LINUX_USERCOPY_H
+#define __LINUX_USERCOPY_H
+
+enum stack_type {
+	BAD_STACK = -1,
+	NOT_STACK = 0,
+	GOOD_FRAME,
+	GOOD_STACK,
+};
+
+#endif
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 3c8da0a..ee7bced 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -16,15 +16,9 @@
 
 #include <linux/mm.h>
 #include <linux/slab.h>
+#include <linux/usercopy.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.7.4

[kernel-hardening] [PATCH v2 2/3] arm64: usercopy: Implement stack frame object validation

[kpark3469]

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

This implements arch_within_stack_frames() for arm64 that should
validate if a given object is contained by a kernel stack frame.

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Reviewed-by: James Morse <james.morse@arm.com>
---
 arch/arm64/Kconfig                   |  1 +
 arch/arm64/include/asm/thread_info.h | 65 ++++++++++++++++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..8bf70b4 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -97,6 +97,7 @@ config ARM64
 	select HAVE_SYSCALL_TRACEPOINTS
 	select HAVE_KPROBES
 	select HAVE_KRETPROBES if HAVE_KPROBES
+	select HAVE_ARCH_WITHIN_STACK_FRAMES
 	select IOMMU_DMA if IOMMU_SUPPORT
 	select IRQ_DOMAIN
 	select IRQ_FORCED_THREADING
diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h
index 46c3b93..4ca7a0b 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -36,6 +36,7 @@
 
 struct task_struct;
 
+#include <linux/usercopy.h>
 #include <asm/stack_pointer.h>
 #include <asm/types.h>
 
@@ -68,7 +69,71 @@ struct thread_info {
 #define thread_saved_fp(tsk)	\
 	((unsigned long)(tsk->thread.cpu_context.fp))
 
+#define get_stack_start(fp) (fp + 2 * sizeof(void *))
+
+/*
+ * Walks up the stack frames to make sure that the specified object is
+ * entirely contained by a single stack frame.
+ *
+ * Returns:
+ *	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 enum stack_type arch_within_stack_frames(const void * const stack,
+					const void * const stackend,
+					const void *obj, unsigned long len)
+{
+#ifdef CONFIG_FRAME_POINTER
+	const void *callee_fp = NULL;
+	const void *caller_fp = NULL;
+
+	callee_fp = __builtin_frame_address(1);
+	if (callee_fp)
+		caller_fp = *(const void * const *)callee_fp;
+	/*
+	 * Case #1:
+	 * low ----------------------------------------------> high
+	 * [callee_fp][lr][args][local vars][caller_fp'][lr']
+	 *                ^----------------^
+	 *               allow copies only within here
+	 *
+	 * Case #2:
+	 * low ----------------------------------------------> high
+	 * [check_object_size_fp][lr][args][local vars][callee_fp][lr]
+	 *                           ^----------------^
+	 *                     dynamically allocated stack variable of
+	 *                     callee frame copies are allowed within here
+	 *
+	 * < example code snippet for Case#2 >
+	 * array_size = get_random_int() & 0x0f;
+	 * if (to_user) {
+	 *         unsigned char array[array_size];
+	 *         if (copy_to_user((void __user *)user_addr, array,
+	 *                          unconst + sizeof(array))) {
+	 */
+	while (stack <= callee_fp && callee_fp < stackend &&
+			!((unsigned long)caller_fp & 0xf)) {
+		/*
+		 * If obj + len extends past the caller frame, this
+		 * check won't pass and the next frame will be 0,
+		 * causing us to bail out and correctly report
+		 * the copy as invalid.
+		 */
+		if (!caller_fp || (obj + len <= caller_fp))
+			return (obj >= get_stack_start(callee_fp)) ?
+				GOOD_FRAME : BAD_STACK;
+		callee_fp = caller_fp;
+		caller_fp = *(const void * const *)caller_fp;
+	}
+	return BAD_STACK;
+#else
+	return NOT_STACK;
 #endif
+}
+
+#endif /* !__ASSEMBLY__ */
 
 /*
  * thread information flags:
-- 
2.7.4

[kernel-hardening] [PATCH v2 3/3] lkdtm: add tests for dynamic array in local stack

[kpark3469]

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

This seems an unusual case in kernel. But, when an array is
dynamically created, this variable can be placed ahead of
current frame pointer. This patch tests this dynamic array case.

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
Reported-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Suggested-by: Kees Cook <keescook@chromium.org>
---
 drivers/misc/lkdtm_usercopy.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/misc/lkdtm_usercopy.c b/drivers/misc/lkdtm_usercopy.c
index 1dd6114..898a323 100644
--- a/drivers/misc/lkdtm_usercopy.c
+++ b/drivers/misc/lkdtm_usercopy.c
@@ -7,6 +7,7 @@
 #include <linux/vmalloc.h>
 #include <linux/mman.h>
 #include <linux/uaccess.h>
+#include <linux/random.h>
 #include <asm/cacheflush.h>
 
 /*
@@ -33,7 +34,14 @@ static noinline unsigned char *trick_compiler(unsigned char *stack)
 
 static noinline unsigned char *do_usercopy_stack_callee(int value)
 {
-	unsigned char buf[32];
+	/*
+	 * buf[128] is big enough to put it in the position ahead of
+	 * check_stack_object()'s frame pointer. Because dynamically allocated
+	 * array can be placed between check_stack_object()'s frame pointer and
+	 * do_usercopy_stack()'s frame pointer, we need an object which exists
+	 * ahead of check_stack_object()'s frame pointer.
+	 */
+	unsigned char buf[128];
 	int i;
 
 	/* Exercise stack to avoid everything living in registers. */
@@ -49,6 +57,7 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 	unsigned long user_addr;
 	unsigned char good_stack[32];
 	unsigned char *bad_stack;
+	unsigned int array_size;
 	int i;
 
 	/* Exercise stack to avoid everything living in registers. */
@@ -72,7 +81,9 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 		return;
 	}
 
+	array_size = get_random_int() & 0x0f;
 	if (to_user) {
+		unsigned char array[array_size];
 		pr_info("attempting good copy_to_user of local stack\n");
 		if (copy_to_user((void __user *)user_addr, good_stack,
 				 unconst + sizeof(good_stack))) {
@@ -80,6 +91,13 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 			goto free_user;
 		}
 
+		pr_info("attempting good copy_to_user of callee stack\n");
+		if (copy_to_user((void __user *)user_addr, array,
+				 unconst + sizeof(array))) {
+			pr_warn("copy_to_user failed unexpectedly?!\n");
+			goto free_user;
+		}
+
 		pr_info("attempting bad copy_to_user of distant stack\n");
 		if (copy_to_user((void __user *)user_addr, bad_stack,
 				 unconst + sizeof(good_stack))) {
@@ -87,6 +105,7 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 			goto free_user;
 		}
 	} else {
+		unsigned char array[array_size];
 		/*
 		 * There isn't a safe way to not be protected by usercopy
 		 * if we're going to write to another thread's stack.
@@ -101,6 +120,13 @@ static noinline void do_usercopy_stack(bool to_user, bool bad_frame)
 			goto free_user;
 		}
 
+		pr_info("attempting good copy_from_user of callee stack\n");
+		if (copy_from_user(array, (void __user *)user_addr,
+				   unconst + sizeof(array))) {
+			pr_warn("copy_from_user failed unexpectedly?!\n");
+			goto free_user;
+		}
+
 		pr_info("attempting bad copy_from_user of distant stack\n");
 		if (copy_from_user(bad_stack, (void __user *)user_addr,
 				   unconst + sizeof(good_stack))) {
-- 
2.7.4

[kernel-hardening] Re: [PATCH v2 1/3] usercopy: create usercopy.h and move enum stack_type

[Kees Cook]

On Thu, Feb 2, 2017 at 4:50 AM,  <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> This patch creates linux/usercopy.h and moves the enum which is
> only used in usercopy.c so as to make it usable to x86 and other
> architecture's thread_info.h, which may have arch_within_stack_frames().

Yeah, I had meant to do this; thanks! See my nit below...

> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
> Suggested-by: James Morse <james.morse@arm.com>
> ---
>  arch/x86/include/asm/thread_info.h | 20 +++++++++++---------
>  include/linux/usercopy.h           | 11 +++++++++++
>  mm/usercopy.c                      |  8 +-------
>  3 files changed, 23 insertions(+), 16 deletions(-)
>  create mode 100644 include/linux/usercopy.h
>
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index ad6f5eb0..c991126 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -51,6 +51,7 @@
>  struct task_struct;
>  #include <asm/cpufeature.h>
>  #include <linux/atomic.h>
> +#include <linux/usercopy.h>
>
>  struct thread_info {
>         unsigned long           flags;          /* low level flags */
> @@ -168,13 +169,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 +198,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/usercopy.h b/include/linux/usercopy.h
> new file mode 100644
> index 0000000..974859e
> --- /dev/null
> +++ b/include/linux/usercopy.h
> @@ -0,0 +1,11 @@
> +#ifndef __LINUX_USERCOPY_H
> +#define __LINUX_USERCOPY_H
> +
> +enum stack_type {
> +       BAD_STACK = -1,
> +       NOT_STACK = 0,
> +       GOOD_FRAME,
> +       GOOD_STACK,
> +};
> +
> +#endif

I think this should live with the declaration of
arch_within_stack_frames in include/linux/thread_info.h instead of
creating a new usercopy.h.

-Kees

> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index 3c8da0a..ee7bced 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -16,15 +16,9 @@
>
>  #include <linux/mm.h>
>  #include <linux/slab.h>
> +#include <linux/usercopy.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.7.4
>



-- 
Kees Cook
Pixel Security