Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLqsPR35kiH2r+DNWpiDayaN21FL6stsQu_kBZJEYcg+w@mail.gmail.com>
Date: Mon, 6 Feb 2017 14:22:15 -0800
From: Kees Cook <keescook@...omium.org>
To: Keun-O Park <kpark3469@...il.com>
Cc: "kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>, 
	Catalin Marinas <catalin.marinas@....com>, Will Deacon <will.deacon@....com>, 
	Mark Rutland <mark.rutland@....com>, James Morse <james.morse@....com>, 
	Pratyush Anand <panand@...hat.com>, keun-o.park@...kmatter.ae
Subject: Re: [PATCH v3 3/3] lkdtm: add tests for dynamic array in local stack

On Sun, Feb 5, 2017 at 4:14 AM,  <kpark3469@...il.com> wrote:
> From: Sahara <keun-o.park@...kmatter.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@...kmatter.ae>
> Reported-by: AKASHI Takahiro <takahiro.akashi@...aro.org>
> Suggested-by: Kees Cook <keescook@...omium.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

Powered by blists - more mailing lists

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