Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to use the new unwinder

[Kees Cook]

On Fri, Aug 12, 2016 at 1:41 PM, Josh Poimboeuf <jpoimboe@xxxxxxxxxx> wrote:
> On Fri, Aug 12, 2016 at 09:29:10AM -0500, Josh Poimboeuf wrote:
>> Convert arch_within_stack_frames() to use the new unwinder.
>>
>> Boot tested with CONFIG_HARDENED_USERCOPY.
>>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
>> ---
>>  arch/x86/lib/usercopy.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
>> index 96ce151..9d0913c 100644
>> --- a/arch/x86/lib/usercopy.c
>> +++ b/arch/x86/lib/usercopy.c
>> @@ -50,12 +50,21 @@ int arch_within_stack_frames(const void * const stack,
>>                            const void * const stackend,
>>                            const void *obj, unsigned long len)
>>  {
>> -     const void *frame = NULL;
>> -     const void *oldframe;
>> +     struct unwind_state state;
>> +     const void *frame, *oldframe;
>> +
>> +     unwind_start(&state, current, NULL, NULL);
>> +
>> +     if (!unwind_next_frame(&state))
>> +             return 0;
>> +
>> +     oldframe = unwind_get_stack_ptr(&state);
>> +
>> +     if (!unwind_next_frame(&state))
>> +             return 0;
>> +
>> +     frame = unwind_get_stack_ptr(&state);
>>
>> -     oldframe = __builtin_frame_address(2);
>> -     if (oldframe)
>> -             frame = __builtin_frame_address(3);
>>       /*
>>        * low ----------------------------------------------> high
>>        * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> @@ -71,8 +80,12 @@ int arch_within_stack_frames(const void * const stack,
>>                */
>>               if (obj + len <= frame)
>>                       return obj >= oldframe + 2 * sizeof(void *) ? 1 : -1;
>> +
>> +             if (!unwind_next_frame(&state))
>> +                     return 0;
>
> I think there's another issue here.  This return needs to be tweaked.
> IIUC, if it reliably reaches the end of the stack without finding the
> object, it should return -1, but if there's something wrong with the
> frame pointers which prevents the unwinder from reaching the end of the
> stack, it should return 0.

Ah, yes, good catch. The callers of this function should have already
determined if the address is outside the stack itself, so this should
only be called when we expect the contents to be somewhere in the
stack. If the unwinder can't find it, then that should be an error,
yes.

-Kees

-- 
Kees Cook
Nexus Security