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