[3/4] arm64: usercopy: consider dynamic array stack variable
diff mbox

Message ID 1519899591-29761-4-git-send-email-kpark3469@gmail.com
State New
Headers show

Commit Message

kpark3469@gmail.com March 1, 2018, 10:19 a.m. UTC
From: Sahara <keun-o.park@darkmatter.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))) {

Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
---
 arch/arm64/kernel/stacktrace.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

Comments

Kees Cook April 4, 2018, 10:57 p.m. UTC | #1
On Thu, Mar 1, 2018 at 2:19 AM,  <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.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?

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

-Kees

[1] https://lkml.org/lkml/2018/3/7/621

>
> Signed-off-by: Sahara <keun-o.park@darkmatter.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
>
kpark3469@gmail.com April 8, 2018, 6:45 a.m. UTC | #2
On Thu, Apr 5, 2018 at 2:57 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 2:19 AM,  <kpark3469@gmail.com> wrote:
>> From: Sahara <keun-o.park@darkmatter.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@chromium.org>
>
> -Kees
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
>>
>> Signed-off-by: Sahara <keun-o.park@darkmatter.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
kpark3469@gmail.com April 9, 2018, 5:54 a.m. UTC | #3
I removed this patch from v2 patchset.
I noticed that this patch can not pass the current lkdtm
stack_frame_to/from tests.
Because the bad_stack is placed before the do_usercopy_stack frame,
this patch makes bad_stack pass the check.
It's not what we want to happen.
For now and later, I think this might be the best solution.

These are the each frame's information in walk_stackframe, when
discard_frame is set to 3 in order to filter the variable length
array.

root@genericarmv8:/sys/kernel/debug/provoke-crash# echo
USERCOPY_STACK_FRAME_TO > DIRECT
[ 3726.088794] lkdtm: Performing direct entry USERCOPY_STACK_FRAME_TO
[ 3726.088867] lkdtm: >>> buf=0xffff00000bf7bc18
[ 3726.088936] lkdtm: >>> do_usercopy_stack_callee:
fp=ffff00000bf7bbf0 xfp=ffff00000bf7bc40
[ 3726.089037] lkdtm: >>> do_usercopy_stack:
good_stack=0xffff00000bf7bc98, bad_stack=0xffff00000bf7bc18
[ 3726.089129] lkdtm: >>> do_usercopy_stack: fp=ffff00000bf7bc40
xfp=ffff00000bf7bcc0
[ 3726.089203] lkdtm: attempting good copy_to_user of local stack
[ 3726.089267] =====================================
[ 3726.089358] > PC        = arch_within_stack_frames+0x0/0x88
[ 3726.089423] > discard_f = 3
[ 3726.089477] > frm_start = 0xffff00000bf7bb90
[ 3726.089539] > frame_end = 0xffff00000bf7bb90
[ 3726.089601] > obj start = 0xffff00000bf7bc98
[ 3726.089662] > obj end   = 0xffff00000bf7bcb8
[ 3726.089720] =====================================
[ 3726.089803] > PC        = check_stack_object+0x44/0x68
[ 3726.089867] > discard_f = 2
[ 3726.089921] > frm_start = 0xffff00000bf7bba0
[ 3726.089982] > frame_end = 0xffff00000bf7bbf0
[ 3726.090044] > obj start = 0xffff00000bf7bc98
[ 3726.090105] > obj end   = 0xffff00000bf7bcb8
[ 3726.090163] =====================================
[ 3726.090247] > PC        = __check_object_size+0x50/0x1e0
[ 3726.090311] > discard_f = 1
[ 3726.090365] > frm_start = 0xffff00000bf7bc00
[ 3726.090427] > frame_end = 0xffff00000bf7bc00
[ 3726.090489] > obj start = 0xffff00000bf7bc98
[ 3726.090550] > obj end   = 0xffff00000bf7bcb8
[ 3726.090608] =====================================
[ 3726.090688] > PC        = do_usercopy_stack+0x188/0x3c0
[ 3726.090752] > discard_f = 0
[ 3726.090806] > frm_start = 0xffff00000bf7bc10
[ 3726.090867] > frame_end = 0xffff00000bf7bc40
[ 3726.091025] > obj start = 0xffff00000bf7bc98
[ 3726.091134] > obj end   = 0xffff00000bf7bcb8
[ 3726.091240] =====================================
[ 3726.091365] > PC        = lkdtm_USERCOPY_STACK_FRAME_TO+0x24/0x38
[ 3726.091495] > discard_f = 0
[ 3726.091596] > frm_start = 0xffff00000bf7bc50
[ 3726.091702] > frame_end = 0xffff00000bf7bcc0
[ 3726.091806] > obj start = 0xffff00000bf7bc98
[ 3726.091909] > obj end   = 0xffff00000bf7bcb8
[ 3726.092037] lkdtm: attempting bad copy_to_user of distant stack
[ 3726.092163] =====================================
[ 3726.092298] > PC        = arch_within_stack_frames+0x0/0x88
[ 3726.092424] > discard_f = 3
[ 3726.092528] > frm_start = 0xffff00000bf7bb90
[ 3726.092634] > frame_end = 0xffff00000bf7bb90
[ 3726.092696] > obj start = 0xffff00000bf7bc18
[ 3726.092757] > obj end   = 0xffff00000bf7bc38
[ 3726.092815] =====================================
[ 3726.092897] > PC        = check_stack_object+0x44/0x68
[ 3726.092960] > discard_f = 2
[ 3726.093014] > frm_start = 0xffff00000bf7bba0
[ 3726.093075] > frame_end = 0xffff00000bf7bbf0
[ 3726.093136] > obj start = 0xffff00000bf7bc18
[ 3726.093198] > obj end   = 0xffff00000bf7bc38
[ 3726.093255] =====================================
[ 3726.093338] > PC        = __check_object_size+0x50/0x1e0
[ 3726.093402] > discard_f = 1
[ 3726.093456] > frm_start = 0xffff00000bf7bc00
[ 3726.093517] > frame_end = 0xffff00000bf7bc00
[ 3726.093578] > obj start = 0xffff00000bf7bc18
[ 3726.093640] > obj end   = 0xffff00000bf7bc38
[ 3726.093697] =====================================
[ 3726.093777] > PC        = do_usercopy_stack+0x318/0x3c0
[ 3726.093840] > discard_f = 0
[ 3726.093894] > frm_start = 0xffff00000bf7bc10
[ 3726.093955] > frame_end = 0xffff00000bf7bc40
[ 3726.094016] > obj start = 0xffff00000bf7bc18
[ 3726.094077] > obj end   = 0xffff00000bf7bc38


Thanks.

BR
Sahara

On Sun, Apr 8, 2018 at 10:45 AM, Keun-O Park <kpark3469@gmail.com> wrote:
> On Thu, Apr 5, 2018 at 2:57 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 2:19 AM,  <kpark3469@gmail.com> wrote:
>>> From: Sahara <keun-o.park@darkmatter.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@chromium.org>
>>
>> -Kees
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>>>
>>> Signed-off-by: Sahara <keun-o.park@darkmatter.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

Patch
diff mbox

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