[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAGXu5jLBc6Ebqzzcfv7Duy5y6_6apfOgZohmyHdSTUAR5xWmsA@mail.gmail.com>
Date: Wed, 4 Apr 2018 16:11:50 -0700
From: Kees Cook <keescook@...omium.org>
To: Keun-O Park <kpark3469@...il.com>
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
[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?
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;
+}
?
> +
> +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?
> +
> /*
> * 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...
> + * 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...
> - 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.
