diff mbox

[v3,3/3] lkdtm: add tests for dynamic array in local stack

Message ID 1486296850-16045-3-git-send-email-kpark3469@gmail.com
State New, archived
Headers show

Commit Message

kpark3469@gmail.com Feb. 5, 2017, 12:14 p.m. UTC
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(-)

Comments

Kees Cook Feb. 6, 2017, 10:22 p.m. UTC | #1
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
diff mbox

Patch

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))) {