linux-kernel - Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 12 Aug 2016 13:47:36 -0700
From:	Kees Cook <keescook@...omium.org>
To:	Josh Poimboeuf <jpoimboe@...hat.com>
Cc:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>,
	"x86@...nel.org" <x86@...nel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Brian Gerst <brgerst@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Byungchul Park <byungchul.park@....com>,
	Nilay Vaish <nilayvaish@...il.com>
Subject: Re: [PATCH v3 51/51] x86/mm: convert arch_within_stack_frames() to
 use the new unwinder

On Fri, Aug 12, 2016 at 1:41 PM, Josh Poimboeuf <jpoimboe@...hat.com> 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@...hat.com>
>> ---
>>  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