[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+KhAHZB6_V-9dKeXzJEmUuYt3Qn7YqoH+rqEncD-tvx9HS6qA@mail.gmail.com>
Date: Sun, 8 Apr 2018 10:45:21 +0400
From: Keun-O Park <kpark3469@...il.com>
To: Kees Cook <keescook@...omium.org>
Cc: Kernel Hardening <kernel-hardening@...ts.openwall.com>, James Morse <james.morse@....com>,
Catalin Marinas <catalin.marinas@....com>, Will Deacon <will.deacon@....com>,
Mark Rutland <mark.rutland@....com>, keun-o.park@...kmatter.ae
Subject: Re: [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable
On Thu, Apr 5, 2018 at 2:57 AM, Kees Cook <keescook@...omium.org> wrote:
> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@...il.com> wrote:
>> From: Sahara <keun-o.park@...kmatter.ae>
>>
>> When an array is dynamically declared, the array may be placed
>> at next frame. If this variable is used for usercopy, then it
>> will cause an Oops because the current check code does not allow
>> this exceptional case.
>>
>> low -----------------------------------------------------> high
>> [__check_object_size fp][lr][args][local vars][caller_fp][lr]
>> ^----------------^
>> dynamically allocated stack variable of
>> caller frame copies are allowed within here
>>
>> < example code snippet >
>> 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))) {
>
> And once we have -Wvla in the build[1] by default we can revert this
> and ignore the VLA case, yes?
Yes, you are right. Once we enable -Wvla, then we don't need this.
Thanks.
BR,
Sahara
>
> Reviewed-by: Kees Cook <keescook@...omium.org>
>
> -Kees
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
>>
>> Signed-off-by: Sahara <keun-o.park@...kmatter.ae>
>> ---
>> arch/arm64/kernel/stacktrace.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 6d37fad..75a8f20 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -162,8 +162,13 @@ int arch_within_stack_frames(const void *stack,
>> * Skip 4 non-inlined frames: <fake frame>,
>> * arch_within_stack_frames(), check_stack_object() and
>> * __check_object_size().
>> + *
>> + * From Akashi's report, an object may be placed between next caller's
>> + * frame, when the object is created as dynamic array.
>> + * Setting the discard_frames to 3 is proper to catch this exceptional
>> + * case.
>> */
>> - arg.discard_frames = 4;
>> + arg.discard_frames = 3;
>>
>> walk_stackframe(current, &frame, check_frame, &arg);
>>
>> --
>> 2.7.4
>>
>
>
>
> --
> 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.
