[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CA+KhAHZTEhjidgfyiixKjfsGs93PS8c1WMNBn8Vi8FaSzfcRHg@mail.gmail.com>
Date: Mon, 9 Apr 2018 09:40:30 +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,
Sodagudi Prasad <psodagud@...eaurora.org>, Josh Poimboeuf <jpoimboe@...hat.com>,
Ingo Molnar <mingo@...nel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames
with unwinder
Hi Kees,
On Thu, Apr 5, 2018 at 3:11 AM, Kees Cook <keescook@...omium.org> wrote:
> [resending with the CCs I forgot...]
>
> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@...il.com> wrote:
>> From: Sahara <keun-o.park@...kmatter.ae>
>>
>> The old arch_within_stack_frames which used the frame pointer is
>> now reimplemented to use frame pointer unwinder apis. So the main
>> functionality is same as before.
>>
>> Signed-off-by: Sahara <keun-o.park@...kmatter.ae>
>
> This will result in slightly more expensive stack checking for
> hardened usercopy, but I think that'd be okay if this could also be
> made to be unwinder-agnostic. Then it would work for ORC too, and
> wouldn't have to depend on just FRAME_POINTER. Without that, I'm not
> sure what the benefit is in changing this?
Exactly. It's the only reason not to depend on the FRAME_POINTER only.
And, it will be better if it would work for ORC.
>
> Further notes below...
>
>> ---
>> arch/x86/include/asm/unwind.h | 5 +++
>> arch/x86/kernel/stacktrace.c | 77 +++++++++++++++++++++++++++++-------------
>> arch/x86/kernel/unwind_frame.c | 4 +--
>> 3 files changed, 60 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h
>> index 1f86e1b..6f04906f 100644
>> --- a/arch/x86/include/asm/unwind.h
>> +++ b/arch/x86/include/asm/unwind.h
>> @@ -87,6 +87,11 @@ void unwind_init(void);
>> void unwind_module_init(struct module *mod, void *orc_ip, size_t orc_ip_size,
>> void *orc, size_t orc_size);
>> #else
>> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
>> +#define FRAME_HEADER_SIZE (sizeof(long) * 2)
>> +size_t regs_size(struct pt_regs *regs);
>> +#endif
>> +
>> static inline void unwind_init(void) {}
>> static inline
>> void unwind_module_init(struct module *mod, void *orc_ip, size_t orc_ip_size,
>> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
>> index f433a33..c26eb55 100644
>> --- a/arch/x86/kernel/stacktrace.c
>> +++ b/arch/x86/kernel/stacktrace.c
>> @@ -12,6 +12,37 @@
>> #include <asm/unwind.h>
>>
>>
>> +static inline void *get_cur_frame(struct unwind_state *state)
>> +{
>> + void *frame = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> + if (state->regs)
>> + frame = (void *)state->regs;
>> + else
>> + frame = (void *)state->bp;
>> +#else
>> +#endif
>> + return frame;
>> +}
>
> What's going on here with the #if statement? Shouldn't this just be:
>
> +static inline void *get_cur_frame(struct unwind_state *state)
> +{
> + void *frame = NULL;
> +
> +#ifdef CONFIG_UNWINDER_FRAME_POINTER
> + if (state->regs)
> + frame = (void *)state->regs;
> + else
> + frame = (void *)state->bp;
> +#endif
> + return frame;
> +}
>
> ?
Removed the unused #ifdef.
>
>> +
>> +static inline void *get_frame_end(struct unwind_state *state)
>> +{
>> + void *frame_end = NULL;
>> +
>> +#if defined(CONFIG_UNWINDER_ORC)
>> +#elif defined(CONFIG_UNWINDER_FRAME_POINTER)
>> + if (state->regs) {
>> + frame_end = (void *)state->regs + regs_size(state->regs);
>> + } else {
>> + frame_end = (void *)state->bp + FRAME_HEADER_SIZE;
>> + }
>> +#else
>> +#endif
>> + return frame_end;
>> +}
>
> Same thing above?
Removed the unused #ifdef.
>
>> +
>> /*
>> * Walks up the stack frames to make sure that the specified object is
>> * entirely contained by a single stack frame.
>> @@ -25,31 +56,31 @@ int arch_within_stack_frames(const void * const stack,
>> const void * const stackend,
>> const void *obj, unsigned long len)
>> {
>> -#if defined(CONFIG_FRAME_POINTER)
>> - const void *frame = NULL;
>> - const void *oldframe;
>> -
>> - oldframe = __builtin_frame_address(2);
>> - if (oldframe)
>> - frame = __builtin_frame_address(3);
>> +#if defined(CONFIG_UNWINDER_FRAME_POINTER)
>> + struct unwind_state state;
>> + void *prev_frame_end = NULL;
>> /*
>> - * low ----------------------------------------------> high
>> - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
>> - * ^----------------^
>> - * allow copies only within here
>
> I think it's worth keeping this diagram: it explains what region is
> being checked...
Kept the comment in v2 patch.
>
>> + * Skip 3 non-inlined frames: arch_within_stack_frames(),
>> + * check_stack_object() and __check_object_size().
>> + *
>> */
>> - while (stack <= frame && frame < stackend) {
>> - /*
>> - * If obj + len extends past the last frame, this
>> - * check won't pass and the next frame will be 0,
>> - * causing us to bail out and correctly report
>> - * the copy as invalid.
>> - */
>
> Also seems like we should keep the comment for describing what's happening...
Kept this comment.
Thanks.
BR,
Sahara
>
>> - if (obj + len <= frame)
>> - return obj >= oldframe + 2 * sizeof(void *) ?
>> - GOOD_FRAME : BAD_STACK;
>> - oldframe = frame;
>> - frame = *(const void * const *)frame;
>> + unsigned int discard_frames = 3;
>> +
>> + for (unwind_start(&state, current, NULL, NULL); !unwind_done(&state);
>> + unwind_next_frame(&state)) {
>> + if (discard_frames) {
>> + discard_frames--;
>> + } else {
>> + void *frame = get_cur_frame(&state);
>> +
>> + if (!frame || !prev_frame_end)
>> + return NOT_STACK;
>> + if (obj + len <= frame)
>> + return obj >= prev_frame_end ?
>> + GOOD_FRAME : BAD_STACK;
>> + }
>> + /* save current frame end before move to next frame */
>> + prev_frame_end = get_frame_end(&state);
>> }
>> return BAD_STACK;
>> #else
>> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
>> index 3dc26f9..c8bfa5c 100644
>> --- a/arch/x86/kernel/unwind_frame.c
>> +++ b/arch/x86/kernel/unwind_frame.c
>> @@ -8,8 +8,6 @@
>> #include <asm/stacktrace.h>
>> #include <asm/unwind.h>
>>
>> -#define FRAME_HEADER_SIZE (sizeof(long) * 2)
>> -
>> unsigned long unwind_get_return_address(struct unwind_state *state)
>> {
>> if (unwind_done(state))
>> @@ -69,7 +67,7 @@ static void unwind_dump(struct unwind_state *state)
>> }
>> }
>>
>> -static size_t regs_size(struct pt_regs *regs)
>> +size_t regs_size(struct pt_regs *regs)
>> {
>> /* x86_32 regs from kernel mode are two words shorter: */
>> if (IS_ENABLED(CONFIG_X86_32) && !user_mode(regs))
>> --
>> 2.7.4
>>
>
> -Kees
>
> --
> Kees Cook
> Pixel Security
>
>
> --
> 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.
