[kernel-hardening] [PATCH v3 1/3] usercopy: create enum stack_type

[kernel-hardening] [PATCH v3 1/3] usercopy: create enum stack_type

[kpark3469]

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>
Suggested-by: James Morse <james.morse@arm.com>
Reviewed-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 ad6f5eb0..7af4b8b 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 5837387..a38b3be 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 3c8da0a..3531ae7 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.7.4

[kernel-hardening] [PATCH v3 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 | 64 ++++++++++++++++++++++++++++++++++++
 2 files changed, 65 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..70baad3 100644
--- a/arch/arm64/include/asm/thread_info.h
+++ b/arch/arm64/include/asm/thread_info.h
@@ -68,7 +68,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 v3 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 v3 3/3] lkdtm: add tests for dynamic array in local stack

[Kees Cook]

On Sun, Feb 5, 2017 at 4:14 AM,  <kpark3469@gmail.com> wrote:
> 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];

I don't understand this change. Why does 32 -> 128 do anything when
it's the address of "buf" that is returned (i.e. the beginning of the
stack buffer).

When check_stack_object() walks the stack, I would expect to see:

[fp][lr][args][local vars][dynamic stack][fp][lr][args][local
vars][dynamic stack]...

Why is a special case needed?

e.g. x86 appears to pass this test correctly already. Maybe I'm not
understanding something about the arm64 stack layout?

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

This can likely just be "array_size = unconst + 8;"

>         if (to_user) {
> +               unsigned char array[array_size];

This needs a blank line after it.

>                 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];

Same needs-blank-line nit.

>                 /*
>                  * 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
>

Thanks for working on this!

-Kees

-- 
Kees Cook
Pixel Security

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

[Kees Cook]

On Sun, Feb 5, 2017 at 4:14 AM,  <kpark3469@gmail.com> wrote:
> 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 | 64 ++++++++++++++++++++++++++++++++++++
>  2 files changed, 65 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..70baad3 100644
> --- a/arch/arm64/include/asm/thread_info.h
> +++ b/arch/arm64/include/asm/thread_info.h
> @@ -68,7 +68,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 *))

This define is only used once: might be better to just leave it
open-coded below.

> +
> +/*
> + * 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 *oldframe = NULL;
> +       const void *frame = NULL;

I've renamed your variables back to match the x86 implementation so I
can more easily compare the differences. Since oldframe is always
initialized, it doesn't need the = NULL above.

> +
> +       oldframe = __builtin_frame_address(1);
> +       if (oldframe)
> +               frame = *(const void * const *)oldframe;

Why this instead of __builting_frame_address(2)? Just to avoid the "2" argument?

> +       /*
> +        * 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

I don't understand how these are different cases. We're walking the
entire stack, so each frame comparison should be the same.

> +        *
> +        * < 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 <= oldframe && oldframe < stackend &&

Why does this examine oldframe instead of the current frame?

> +                       !((unsigned long)frame & 0xf)) {

What is the 0xf test?

> +               /*
> +                * 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 (!frame || (obj + len <= frame))

Excepting the initial frame test, this is identical to x86, which seems fine.

> +                       return (obj >= oldframe + 2 * sizeof(void *) ?
> +                               GOOD_FRAME : BAD_STACK;
> +               oldframe = frame;
> +               frame = *(const void * const *)frame;
> +       }
> +       return BAD_STACK;
> +#else
> +       return NOT_STACK;
>  #endif
> +}
> +
> +#endif /* !__ASSEMBLY__ */
>
>  /*
>   * thread information flags:
> --
> 2.7.4
>

I guess I just don't see the special case for arm64 frames?

-Kees

-- 
Kees Cook
Pixel Security

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

[James Morse]

On 05/02/17 12:14, kpark3469@gmail.com wrote:
> 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>

Careful, you should only include tags like this when they are explicitly given.
I don't remember doing that, and don't see it here:
http://www.openwall.com/lists/kernel-hardening/2017/01/26/8

I'd like to avoid having two sets of code that walk the stack.
I will have a go at a version of this patch that uses arm64s existing
walk_stackframe() machinery - lets find out if there is a good reason not to do
it that way!


Thanks,

James

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

[James Morse]

Hi Sahara,

On 07/02/17 10:19, James Morse wrote:
> On 05/02/17 12:14, kpark3469@gmail.com wrote:
>> 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>

> I'd like to avoid having two sets of code that walk the stack.
> I will have a go at a version of this patch that uses arm64s existing
> walk_stackframe() machinery - lets find out if there is a good reason not to do
> it that way!

The reason turns out to be because LKDTM isn't testing whether we are
overlapping stack frames.
Instead it wants us to tell it whether the original caller somewhere down the
stack pointed into a stack frame that hadn't yet been written. This requires
this function to know how it will be called and unwind some number of frames.
Annoyingly we have to maintain start/end boundaries for each frame in case the
object was neatly contained in a frame that wasn't written at the time.

Ideally we would inline this stack check into the caller, testing object_end
doesn't lie in the range current_stack_pointer : end_of_stack. This would work
even when built without CONFIG_FRAME_POINTER but is probably too-invasive a change.

As is, the walk_stackframe() machinery version looks like this:
---------------------------------------%<---------------------------------------
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 8a552a33c6ef..620fa74201b5 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,73 @@ 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;
+       unsigned long frame_start;
+       int discard_frames;
+       int err;
+};
+
+static int check_frame(struct stackframe *frame, void *d)
+{
+       struct check_frame_arg *arg = d;
+       unsigned long frame_end = frame->fp;
+
+       /* object overlaps multiple frames */
+       if (arg->obj_start < frame->fp && frame->fp < arg->obj_end) {
+               arg->err = BAD_STACK;
+               return 1;
+       }
+
+       /*
+        * Discard frames and check object is in a frame written early
+        * enough.
+        */
+       if (arg->discard_frames)
+               arg->discard_frames--;
+       else if ((arg->frame_start <= arg->obj_start && arg->obj_start <
frame_end) &&
+                (arg->frame_start < arg->obj_end && arg->obj_end <= frame_end))
+               return 1;
+
+       /* object exists in a previous frame */
+       if (arg->obj_end < arg->frame_start) {
+               arg->err = BAD_STACK;
+               return 1;
+       }
+
+       arg->frame_start = frame_end + 0x10;
+
+       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);
+       arg.frame_start = frame.fp;
+
+       /*
+        * Skip 4 non-inlined frames: <fake frame>,
+        * arch_within_stack_frames(), check_stack_object() and
+        * __check_object_size().
+        */
+       arg.discard_frames = 4;
+
+       walk_stackframe(current, &frame, check_frame, &arg);
+
+       return arg.err;
+}
---------------------------------------%<---------------------------------------

This avoids the __builtin_frame_address(!0) problem and doesn't duplicate the
actual stack walking. This can be simplified further if we can get rid of the
time-travel requirements.

It is unfortunately more code, but it is hopefully clearer!


Thanks,

James

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

[Kees Cook]

On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote:
> Hi Sahara,
>
> On 07/02/17 10:19, James Morse wrote:
>> On 05/02/17 12:14, kpark3469@gmail.com wrote:
>>> 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>
>
>> I'd like to avoid having two sets of code that walk the stack.
>> I will have a go at a version of this patch that uses arm64s existing
>> walk_stackframe() machinery - lets find out if there is a good reason not to do
>> it that way!
>
> The reason turns out to be because LKDTM isn't testing whether we are
> overlapping stack frames.
> Instead it wants us to tell it whether the original caller somewhere down the
> stack pointed into a stack frame that hadn't yet been written. This requires
> this function to know how it will be called and unwind some number of frames.
> Annoyingly we have to maintain start/end boundaries for each frame in case the
> object was neatly contained in a frame that wasn't written at the time.

"hadn't yet been written"? This doesn't make sense. The hardened
usercopy stack frame check (which is what LKDTM is exercising) wants
to simply walk from the current frame up, making sure that the object
in question is entirely contained by any single stack frame. Any
dynamic stack allocations should already be covered since it would be
within the caller's frame.

Am I missing something?

> Ideally we would inline this stack check into the caller, testing object_end
> doesn't lie in the range current_stack_pointer : end_of_stack. This would work
> even when built without CONFIG_FRAME_POINTER but is probably too-invasive a change.
>
> As is, the walk_stackframe() machinery version looks like this:
> ---------------------------------------%<---------------------------------------
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 8a552a33c6ef..620fa74201b5 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,73 @@ 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;
> +       unsigned long frame_start;
> +       int discard_frames;
> +       int err;
> +};
> +
> +static int check_frame(struct stackframe *frame, void *d)
> +{
> +       struct check_frame_arg *arg = d;
> +       unsigned long frame_end = frame->fp;
> +
> +       /* object overlaps multiple frames */
> +       if (arg->obj_start < frame->fp && frame->fp < arg->obj_end) {
> +               arg->err = BAD_STACK;
> +               return 1;
> +       }
> +
> +       /*
> +        * Discard frames and check object is in a frame written early
> +        * enough.
> +        */
> +       if (arg->discard_frames)
> +               arg->discard_frames--;
> +       else if ((arg->frame_start <= arg->obj_start && arg->obj_start <
> frame_end) &&
> +                (arg->frame_start < arg->obj_end && arg->obj_end <= frame_end))
> +               return 1;
> +
> +       /* object exists in a previous frame */
> +       if (arg->obj_end < arg->frame_start) {
> +               arg->err = BAD_STACK;
> +               return 1;
> +       }
> +
> +       arg->frame_start = frame_end + 0x10;
> +
> +       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);
> +       arg.frame_start = frame.fp;
> +
> +       /*
> +        * Skip 4 non-inlined frames: <fake frame>,
> +        * arch_within_stack_frames(), check_stack_object() and
> +        * __check_object_size().
> +        */
> +       arg.discard_frames = 4;
> +
> +       walk_stackframe(current, &frame, check_frame, &arg);
> +
> +       return arg.err;
> +}
> ---------------------------------------%<---------------------------------------
>
> This avoids the __builtin_frame_address(!0) problem and doesn't duplicate the
> actual stack walking. This can be simplified further if we can get rid of the
> time-travel requirements.
>
> It is unfortunately more code, but it is hopefully clearer!

This doesn't seem to sanity-check that the frame is still within the
process stack. We'd want to make sure it can't walk off into la-la
land. :) (We could just add "stack" and "stack_end" to the
check_frame_arg struct along with checks?)

-Kees

-- 
Kees Cook
Pixel Security

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

[James Morse]

Hi Kees,

On 07/02/17 18:13, Kees Cook wrote:
> On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote:
>> On 07/02/17 10:19, James Morse wrote:
>> The reason turns out to be because LKDTM isn't testing whether we are
>> overlapping stack frames.
>> Instead it wants us to tell it whether the original caller somewhere down the
>> stack pointed into a stack frame that hadn't yet been written. This requires
>> this function to know how it will be called and unwind some number of frames.
>> Annoyingly we have to maintain start/end boundaries for each frame in case the
>> object was neatly contained in a frame that wasn't written at the time.
> 
> "hadn't yet been written"? This doesn't make sense.

Sorry, "wasn't contained by a frame at the time copy_to_user() was called, even
if it is now...".


> The hardened
> usercopy stack frame check (which is what LKDTM is exercising) wants
> to simply walk from the current frame up, making sure that the object
> in question is entirely contained by any single stack frame. Any
> dynamic stack allocations should already be covered since it would be
> within the caller's frame.

Sure, maybe I'm looking at the wrong lkdtm test then. I see this happening:

do_usercopy_stack_callee() returns its own stack value (while trying to confuse
the compiler). We know this value must be after do_usercopy_stack()s frame.
do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test
expects this to to be rejected.

copy_{to,from}_user() then inline a call to __check_object_size(), which in turn
calls check_stack_object() (which is marked noinline). These calls will generate
stack frames, which will overlap the value do_usercopy_stack_callee() returned.

By the time arch_within_stack_frames() is called, the value returned by
do_usercopy_stack_callee() is within a stack frame. It just wasn't within a
stack frame at the time copy_to_user() was called.

Does this make sense, or have I gone off the rails?


One way to fix this is to make the size given to copy_to_user() so large that it
must overlap multiple stack frames. 32 bytes is too small given arm64 kernel
stacks have to be 16 byte aligned.

A better trick would be to inline the 'not after our stack frame' check into
do_usercopy_stack(), but that means exposing the report_usercopy() and maybe
some more. (I will give it a go).


> This doesn't seem to sanity-check that the frame is still within the
> process stack. We'd want to make sure it can't walk off into la-la
> land. :) (We could just add "stack" and "stack_end" to the
> check_frame_arg struct along with checks?)

The arch unwind_frame() machinery does this for us, in particular the cryptic:
>	if (fp < low || fp > high || fp & 0xf)
>		return -EINVAL;

Is testing that the freshly read 'fp' is between the 'top' of this frame and the
'bottom' of the stack.

The only corner case would be if you called this and object wasn't on the stack
at all to begin with, but core code already checks this. Before calling
arch_within_stack_frames(),
mm/usercopy.c:check_stack_object():
>	/* Object is not on the stack at all. */
>	if (obj + len <= stack || stackend <= obj)
>		return NOT_STACK;



Thanks,

James

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

[Kees Cook]

On Wed, Feb 8, 2017 at 3:16 AM, James Morse <james.morse@arm.com> wrote:
> On 07/02/17 18:13, Kees Cook wrote:
>> On Tue, Feb 7, 2017 at 9:03 AM, James Morse <james.morse@arm.com> wrote:
>>> On 07/02/17 10:19, James Morse wrote:
>>> The reason turns out to be because LKDTM isn't testing whether we are
>>> overlapping stack frames.
>>> Instead it wants us to tell it whether the original caller somewhere down the
>>> stack pointed into a stack frame that hadn't yet been written. This requires
>>> this function to know how it will be called and unwind some number of frames.
>>> Annoyingly we have to maintain start/end boundaries for each frame in case the
>>> object was neatly contained in a frame that wasn't written at the time.
>>
>> "hadn't yet been written"? This doesn't make sense.
>
> Sorry, "wasn't contained by a frame at the time copy_to_user() was called, even
> if it is now...".
>
>> The hardened
>> usercopy stack frame check (which is what LKDTM is exercising) wants
>> to simply walk from the current frame up, making sure that the object
>> in question is entirely contained by any single stack frame. Any
>> dynamic stack allocations should already be covered since it would be
>> within the caller's frame.
>
> Sure, maybe I'm looking at the wrong lkdtm test then. I see this happening:
>
> do_usercopy_stack_callee() returns its own stack value (while trying to confuse
> the compiler). We know this value must be after do_usercopy_stack()s frame.
> do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test
> expects this to to be rejected.
>
> copy_{to,from}_user() then inline a call to __check_object_size(), which in turn
> calls check_stack_object() (which is marked noinline). These calls will generate
> stack frames, which will overlap the value do_usercopy_stack_callee() returned.
>
> By the time arch_within_stack_frames() is called, the value returned by
> do_usercopy_stack_callee() is within a stack frame. It just wasn't within a
> stack frame at the time copy_to_user() was called.
>
> Does this make sense, or have I gone off the rails?

That's true, but those frames should be ignored by the walker, and as
such, should be rejected. (See below.)

> One way to fix this is to make the size given to copy_to_user() so large that it
> must overlap multiple stack frames. 32 bytes is too small given arm64 kernel
> stacks have to be 16 byte aligned.
>
> A better trick would be to inline the 'not after our stack frame' check into
> do_usercopy_stack(), but that means exposing the report_usercopy() and maybe
> some more. (I will give it a go).

Just to make sure I'm on the same page, the call stack is:

do_usercopy_stack() (or anything calling the uaccess functions)
  copy_{to,from}_user() <- inlined into do_usercopy_stack()
__check_object_size()
check_stack_object()
  arch_within_stack_frames() <- inlined into check_stack_object()

So, really, arch_within_stack_frames() should ignore the current frame
(from check_stack_object()) and prior frame (from
__check_object_size()), before starting its examination of frames.
This is what the x86 walker does:

        oldframe = __builtin_frame_address(1);
        if (oldframe)
                frame = __builtin_frame_address(2);

frame address 0 would be check_stack_object(), 1 would be
__check_object_size(), so it starts its analysis at frame 2, which is
do_usercopy_stack()'s frame, bounded by the end of
__check_object_size()'s frame.

Is there any reason the arm64 walker couldn't be identical to the x86 walker?

>> This doesn't seem to sanity-check that the frame is still within the
>> process stack. We'd want to make sure it can't walk off into la-la
>> land. :) (We could just add "stack" and "stack_end" to the
>> check_frame_arg struct along with checks?)
>
> The arch unwind_frame() machinery does this for us, in particular the cryptic:
>>       if (fp < low || fp > high || fp & 0xf)
>>               return -EINVAL;
>
> Is testing that the freshly read 'fp' is between the 'top' of this frame and the
> 'bottom' of the stack.
>
> The only corner case would be if you called this and object wasn't on the stack
> at all to begin with, but core code already checks this. Before calling
> arch_within_stack_frames(),
> mm/usercopy.c:check_stack_object():
>>       /* Object is not on the stack at all. */
>>       if (obj + len <= stack || stackend <= obj)
>>               return NOT_STACK;

Cool, yeah.

-Kees

-- 
Kees Cook
Pixel Security

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

[James Morse]

Hi Kees,

On 08/02/17 21:38, Kees Cook wrote:
> On Wed, Feb 8, 2017 at 3:16 AM, James Morse <james.morse@arm.com> wrote:
>> do_usercopy_stack_callee() returns its own stack value (while trying to confuse
>> the compiler). We know this value must be after do_usercopy_stack()s frame.
>> do_usercopy_stack() then passes this value to copy_{to,from}_user(), the test
>> expects this to to be rejected.
>>
>> copy_{to,from}_user() then inline a call to __check_object_size(), which in turn
>> calls check_stack_object() (which is marked noinline). These calls will generate
>> stack frames, which will overlap the value do_usercopy_stack_callee() returned.
>>
>> By the time arch_within_stack_frames() is called, the value returned by
>> do_usercopy_stack_callee() is within a stack frame. It just wasn't within a
>> stack frame at the time copy_to_user() was called.
>>
>> Does this make sense, or have I gone off the rails?
> 
> That's true, but those frames should be ignored by the walker, and as
> such, should be rejected. (See below.)

I think that's an odd thing for arch_within_stack_frames() to be doing.


>> One way to fix this is to make the size given to copy_to_user() so large that it
>> must overlap multiple stack frames. 32 bytes is too small given arm64 kernel
>> stacks have to be 16 byte aligned.
>>
>> A better trick would be to inline the 'not after our stack frame' check into
>> do_usercopy_stack(), but that means exposing the report_usercopy() and maybe
>> some more. (I will give it a go).
> 
> Just to make sure I'm on the same page, the call stack is:
> 
> do_usercopy_stack() (or anything calling the uaccess functions)
>   copy_{to,from}_user() <- inlined into do_usercopy_stack()
> __check_object_size()
> check_stack_object()
>   arch_within_stack_frames() <- inlined into check_stack_object()

I think this is where our world-view is different, I don't trust the compiler
not to pull some surprising optimisation that inlines calls differently at
different call-sites.

The compiler won't always inline functions marked inline, [0] has some examples,
(I'm not sure what it means by 'use of nested functions'!).

Expecting a particular layout is fragile, Akashi's example shows gcc doesn't
always place objects and the frame record where we expected. Requiring a
particular layout for copy_to_user() to work is bordering on the 'sleepless
nights' territory.


> Is there any reason the arm64 walker couldn't be identical to the x86 walker?

We would then have two stack walkers.

In the light of Akashi's example, walking the stack and saying 'this object was
allocated by this call' isn't something we can do, arch_within_stack_frames()
shouldn't try.

I have an alternate version of this patch that uses arm64s existing stack walker
to look for fp appearing within an object, and another that tries to inline the
bounds check into the caller. I will post these shortly for comparison...


Thanks,

James


[0] https://gcc.gnu.org/onlinedocs/gcc/Inline.html

[kernel-hardening] Re: [PATCH v3 1/3] usercopy: create enum stack_type

[Kees Cook]

On Sun, Feb 5, 2017 at 4:14 AM,  <kpark3469@gmail.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>
> Suggested-by: James Morse <james.morse@arm.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>

Acked-by: Kees Cook <keescook@chromium.org>

-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 ad6f5eb0..7af4b8b 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 5837387..a38b3be 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 3c8da0a..3531ae7 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.7.4
>



-- 
Kees Cook
Pixel Security