* Re: [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h
[not found] ` <1519899591-29761-2-git-send-email-kpark3469@gmail.com>
@ 2018-04-04 22:52 ` Kees Cook
[not found] ` <1519899591-29761-3-git-send-email-kpark3469@gmail.com>
1 sibling, 0 replies; 5+ messages in thread
From: Kees Cook @ 2018-04-04 22:52 UTC (permalink / raw)
To: Keun-O Park
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park, Sodagudi Prasad, Josh Poimboeuf,
Ingo Molnar, LKML
On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> Since the inlined arch_within_stack_frames function was placed within
> asm/thread_info.h, using stacktrace functions to unwind stack was
> restricted. Now in order to have this function use more abundant apis,
> it is moved to architecture's stacktrace.c. And, it is changed from
> inline to noinline function to clearly have three depth calls like:
>
> do_usercopy_stack()
> copy_[to|from]_user() : inline
> check_copy_size() : inline
> __check_object_size()
> check_stack_object()
> arch_within_stack_frames()
>
> With this change, the x86's implementation was slightly changed also.
> And, linux/stacktrace.h includes asm/stacktrace.h from now on.
>
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
This seems like a good clean-up regardless of anything else. :)
I think x86 folks, especially Josh Poimboeuf and Ingo Molnar, and LKML
should be included in CC for follow-up versions of this series, since
it touches the x86 stuff too.
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> arch/arm/kernel/stacktrace.c | 1 -
> arch/arm64/kernel/stacktrace.c | 1 -
> arch/cris/kernel/stacktrace.c | 1 -
> arch/metag/kernel/stacktrace.c | 2 --
> arch/mips/kernel/stacktrace.c | 1 -
> arch/sh/kernel/stacktrace.c | 1 -
> arch/sparc/kernel/stacktrace.c | 1 -
> arch/um/kernel/stacktrace.c | 1 -
> arch/unicore32/kernel/process.c | 1 -
> arch/unicore32/kernel/stacktrace.c | 2 --
> arch/x86/include/asm/thread_info.h | 51 +-------------------------------------
> arch/x86/kernel/Makefile | 2 +-
> arch/x86/kernel/stacktrace.c | 50 ++++++++++++++++++++++++++++++++++++-
> arch/xtensa/kernel/stacktrace.c | 1 -
> include/linux/page_ext.h | 1 -
> include/linux/stacktrace.h | 25 +++++++++++++++++++
> include/linux/thread_info.h | 21 ----------------
> kernel/sysctl.c | 1 -
> mm/usercopy.c | 2 +-
> 19 files changed, 77 insertions(+), 89 deletions(-)
>
> diff --git a/arch/arm/kernel/stacktrace.c b/arch/arm/kernel/stacktrace.c
> index a56e7c8..d7d629b 100644
> --- a/arch/arm/kernel/stacktrace.c
> +++ b/arch/arm/kernel/stacktrace.c
> @@ -4,7 +4,6 @@
> #include <linux/stacktrace.h>
>
> #include <asm/sections.h>
> -#include <asm/stacktrace.h>
> #include <asm/traps.h>
>
> #if defined(CONFIG_FRAME_POINTER) && !defined(CONFIG_ARM_UNWIND)
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 76809cc..fbc366d 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -25,7 +25,6 @@
>
> #include <asm/irq.h>
> #include <asm/stack_pointer.h>
> -#include <asm/stacktrace.h>
>
> /*
> * AArch64 PCS assigns the frame pointer to x29.
> diff --git a/arch/cris/kernel/stacktrace.c b/arch/cris/kernel/stacktrace.c
> index f1cc3aa..aae4196 100644
> --- a/arch/cris/kernel/stacktrace.c
> +++ b/arch/cris/kernel/stacktrace.c
> @@ -1,7 +1,6 @@
> #include <linux/sched.h>
> #include <linux/sched/debug.h>
> #include <linux/stacktrace.h>
> -#include <asm/stacktrace.h>
>
> void walk_stackframe(unsigned long sp,
> int (*fn)(unsigned long addr, void *data),
> diff --git a/arch/metag/kernel/stacktrace.c b/arch/metag/kernel/stacktrace.c
> index 09d67b7..9502a29 100644
> --- a/arch/metag/kernel/stacktrace.c
> +++ b/arch/metag/kernel/stacktrace.c
> @@ -4,8 +4,6 @@
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
>
> -#include <asm/stacktrace.h>
> -
> #if defined(CONFIG_FRAME_POINTER)
>
> #ifdef CONFIG_KALLSYMS
> diff --git a/arch/mips/kernel/stacktrace.c b/arch/mips/kernel/stacktrace.c
> index 7c7c902..0b44e10 100644
> --- a/arch/mips/kernel/stacktrace.c
> +++ b/arch/mips/kernel/stacktrace.c
> @@ -8,7 +8,6 @@
> #include <linux/sched/task_stack.h>
> #include <linux/stacktrace.h>
> #include <linux/export.h>
> -#include <asm/stacktrace.h>
>
> /*
> * Save stack-backtrace addresses into a stack_trace buffer:
> diff --git a/arch/sh/kernel/stacktrace.c b/arch/sh/kernel/stacktrace.c
> index 7a73d27..7a7ac4c 100644
> --- a/arch/sh/kernel/stacktrace.c
> +++ b/arch/sh/kernel/stacktrace.c
> @@ -16,7 +16,6 @@
> #include <linux/module.h>
> #include <asm/unwinder.h>
> #include <asm/ptrace.h>
> -#include <asm/stacktrace.h>
>
> static int save_stack_stack(void *data, char *name)
> {
> diff --git a/arch/sparc/kernel/stacktrace.c b/arch/sparc/kernel/stacktrace.c
> index be4c14c..42cdf86 100644
> --- a/arch/sparc/kernel/stacktrace.c
> +++ b/arch/sparc/kernel/stacktrace.c
> @@ -5,7 +5,6 @@
> #include <linux/ftrace.h>
> #include <linux/export.h>
> #include <asm/ptrace.h>
> -#include <asm/stacktrace.h>
>
> #include "kstack.h"
>
> diff --git a/arch/um/kernel/stacktrace.c b/arch/um/kernel/stacktrace.c
> index ebe7bcf..a0d556e 100644
> --- a/arch/um/kernel/stacktrace.c
> +++ b/arch/um/kernel/stacktrace.c
> @@ -14,7 +14,6 @@
> #include <linux/stacktrace.h>
> #include <linux/module.h>
> #include <linux/uaccess.h>
> -#include <asm/stacktrace.h>
>
> void dump_trace(struct task_struct *tsk,
> const struct stacktrace_ops *ops,
> diff --git a/arch/unicore32/kernel/process.c b/arch/unicore32/kernel/process.c
> index 2bc10b8..ffca651 100644
> --- a/arch/unicore32/kernel/process.c
> +++ b/arch/unicore32/kernel/process.c
> @@ -36,7 +36,6 @@
>
> #include <asm/cacheflush.h>
> #include <asm/processor.h>
> -#include <asm/stacktrace.h>
>
> #include "setup.h"
>
> diff --git a/arch/unicore32/kernel/stacktrace.c b/arch/unicore32/kernel/stacktrace.c
> index 9976e76..f9cd2a4 100644
> --- a/arch/unicore32/kernel/stacktrace.c
> +++ b/arch/unicore32/kernel/stacktrace.c
> @@ -14,8 +14,6 @@
> #include <linux/sched/debug.h>
> #include <linux/stacktrace.h>
>
> -#include <asm/stacktrace.h>
> -
> #if defined(CONFIG_FRAME_POINTER)
> /*
> * Unwind the current stack frame and store the new register values in the
> diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
> index a5d9521..e25d70a 100644
> --- a/arch/x86/include/asm/thread_info.h
> +++ b/arch/x86/include/asm/thread_info.h
> @@ -156,59 +156,10 @@ struct thread_info {
> *
> * preempt_count needs to be 1 initially, until the scheduler is functional.
> */
> -#ifndef __ASSEMBLY__
> -
> -/*
> - * Walks up the stack frames to make sure that the specified object is
> - * entirely contained by a single stack frame.
> - *
> - * Returns:
> - * GOOD_FRAME if within a frame
> - * BAD_STACK if placed across a frame boundary (or outside stack)
> - * NOT_STACK unable to determine (no frame pointers, etc)
> - */
> -static inline 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(1);
> - if (oldframe)
> - frame = __builtin_frame_address(2);
> - /*
> - * low ----------------------------------------------> high
> - * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> - * ^----------------^
> - * allow copies only within here
> - */
> - 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.
> - */
> - if (obj + len <= frame)
> - return obj >= oldframe + 2 * sizeof(void *) ?
> - GOOD_FRAME : BAD_STACK;
> - oldframe = frame;
> - frame = *(const void * const *)frame;
> - }
> - return BAD_STACK;
> -#else
> - return NOT_STACK;
> -#endif
> -}
> -
> -#else /* !__ASSEMBLY__ */
> -
> +#ifdef __ASSEMBLY__
> #ifdef CONFIG_X86_64
> # define cpu_current_top_of_stack (cpu_tss_rw + TSS_sp1)
> #endif
> -
> #endif
>
> #ifdef CONFIG_COMPAT
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 29786c8..3a906c3 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -70,7 +70,7 @@ obj-$(CONFIG_IA32_EMULATION) += tls.o
> obj-y += step.o
> obj-$(CONFIG_INTEL_TXT) += tboot.o
> obj-$(CONFIG_ISA_DMA_API) += i8237.o
> -obj-$(CONFIG_STACKTRACE) += stacktrace.o
> +obj-y += stacktrace.o
> obj-y += cpu/
> obj-y += acpi/
> obj-y += reboot.o
> diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c
> index 093f2ea..f433a33 100644
> --- a/arch/x86/kernel/stacktrace.c
> +++ b/arch/x86/kernel/stacktrace.c
> @@ -9,9 +9,56 @@
> #include <linux/stacktrace.h>
> #include <linux/export.h>
> #include <linux/uaccess.h>
> -#include <asm/stacktrace.h>
> #include <asm/unwind.h>
>
> +
> +/*
> + * Walks up the stack frames to make sure that the specified object is
> + * entirely contained by a single stack frame.
> + *
> + * Returns:
> + * GOOD_FRAME if within a frame
> + * BAD_STACK if placed across a frame boundary (or outside stack)
> + * NOT_STACK unable to determine (no frame pointers, etc)
> + */
> +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);
> + /*
> + * low ----------------------------------------------> high
> + * [saved bp][saved ip][args][local vars][saved bp][saved ip]
> + * ^----------------^
> + * allow copies only within here
> + */
> + 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.
> + */
> + if (obj + len <= frame)
> + return obj >= oldframe + 2 * sizeof(void *) ?
> + GOOD_FRAME : BAD_STACK;
> + oldframe = frame;
> + frame = *(const void * const *)frame;
> + }
> + return BAD_STACK;
> +#else
> + return NOT_STACK;
> +#endif
> +}
> +
> +#ifdef CONFIG_STACKTRACE
> +
> static int save_stack_address(struct stack_trace *trace, unsigned long addr,
> bool nosched)
> {
> @@ -241,3 +288,4 @@ void save_stack_trace_user(struct stack_trace *trace)
> if (trace->nr_entries < trace->max_entries)
> trace->entries[trace->nr_entries++] = ULONG_MAX;
> }
> +#endif /* CONFIG_STACKTRACE */
> diff --git a/arch/xtensa/kernel/stacktrace.c b/arch/xtensa/kernel/stacktrace.c
> index 0df4080..ab831d8 100644
> --- a/arch/xtensa/kernel/stacktrace.c
> +++ b/arch/xtensa/kernel/stacktrace.c
> @@ -12,7 +12,6 @@
> #include <linux/sched.h>
> #include <linux/stacktrace.h>
>
> -#include <asm/stacktrace.h>
> #include <asm/traps.h>
> #include <linux/uaccess.h>
>
> diff --git a/include/linux/page_ext.h b/include/linux/page_ext.h
> index ca5461e..f3b7dd9 100644
> --- a/include/linux/page_ext.h
> +++ b/include/linux/page_ext.h
> @@ -3,7 +3,6 @@
> #define __LINUX_PAGE_EXT_H
>
> #include <linux/types.h>
> -#include <linux/stacktrace.h>
> #include <linux/stackdepot.h>
>
> struct pglist_data;
> diff --git a/include/linux/stacktrace.h b/include/linux/stacktrace.h
> index ba29a06..4fd061e 100644
> --- a/include/linux/stacktrace.h
> +++ b/include/linux/stacktrace.h
> @@ -3,10 +3,35 @@
> #define __LINUX_STACKTRACE_H
>
> #include <linux/types.h>
> +#include <asm/stacktrace.h>
>
> struct task_struct;
> struct pt_regs;
>
> +/*
> + * For per-arch arch_within_stack_frames() implementations, defined in
> + * kernel/stacktrace.c.
> + */
> +enum {
> + BAD_STACK = -1,
> + NOT_STACK = 0,
> + GOOD_FRAME,
> + GOOD_STACK,
> +};
> +
> +#ifdef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
> +extern int arch_within_stack_frames(const void * const stack,
> + const void * const stackend,
> + const void *obj, unsigned long len);
> +#else
> +static inline int arch_within_stack_frames(const void * const stack,
> + const void * const stackend,
> + const void *obj, unsigned long len)
> +{
> + return NOT_STACK;
> +}
> +#endif
> +
> #ifdef CONFIG_STACKTRACE
> struct stack_trace {
> unsigned int nr_entries, max_entries;
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 34f053a..5403851 100644
> --- a/include/linux/thread_info.h
> +++ b/include/linux/thread_info.h
> @@ -23,18 +23,6 @@
> #endif
>
> #include <linux/bitops.h>
> -
> -/*
> - * For per-arch arch_within_stack_frames() implementations, defined in
> - * asm/thread_info.h.
> - */
> -enum {
> - BAD_STACK = -1,
> - NOT_STACK = 0,
> - GOOD_FRAME,
> - GOOD_STACK,
> -};
> -
> #include <asm/thread_info.h>
>
> #ifdef __KERNEL__
> @@ -92,15 +80,6 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
>
> #define tif_need_resched() test_thread_flag(TIF_NEED_RESCHED)
>
> -#ifndef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
> -static inline int arch_within_stack_frames(const void * const stack,
> - const void * const stackend,
> - const void *obj, unsigned long len)
> -{
> - return 0;
> -}
> -#endif
> -
> #ifdef CONFIG_HARDENED_USERCOPY
> extern void __check_object_size(const void *ptr, unsigned long n,
> bool to_user);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 2fb4e27..a1ee965 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -73,7 +73,6 @@
>
> #ifdef CONFIG_X86
> #include <asm/nmi.h>
> -#include <asm/stacktrace.h>
> #include <asm/io.h>
> #endif
> #ifdef CONFIG_SPARC
> diff --git a/mm/usercopy.c b/mm/usercopy.c
> index e9e9325..6a74776 100644
> --- a/mm/usercopy.c
> +++ b/mm/usercopy.c
> @@ -19,7 +19,7 @@
> #include <linux/sched.h>
> #include <linux/sched/task.h>
> #include <linux/sched/task_stack.h>
> -#include <linux/thread_info.h>
> +#include <linux/stacktrace.h>
> #include <asm/sections.h>
>
> /*
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames
[not found] ` <1519899591-29761-3-git-send-email-kpark3469@gmail.com>
@ 2018-04-04 22:55 ` Kees Cook
2018-04-08 6:39 ` Keun-O Park
[not found] ` <1519899591-29761-4-git-send-email-kpark3469@gmail.com>
1 sibling, 1 reply; 5+ messages in thread
From: Kees Cook @ 2018-04-04 22:55 UTC (permalink / raw)
To: Keun-O Park
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park, Sodagudi Prasad, Josh Poimboeuf,
Ingo Molnar, LKML
On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
> From: James Morse <james.morse@arm.com>
>
> This implements arch_within_stack_frames() for arm64 that should
> validate if a given object is contained by a kernel stack frame.
>
> Signed-off-by: James Morse <james.morse@arm.com>
> Reviewed-by: Sahara <keun-o.park@darkmatter.ae>
Looks good to me. Does this end up passing the LKDTM
USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests?
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
> ---
> arch/arm64/Kconfig | 1 +
> arch/arm64/kernel/stacktrace.c | 76 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 77 insertions(+)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 5361287..b6c3b52 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -127,6 +127,7 @@ config ARM64
> select HAVE_SYSCALL_TRACEPOINTS
> select HAVE_KPROBES
> select HAVE_KRETPROBES
> + select HAVE_ARCH_WITHIN_STACK_FRAMES
> select IOMMU_DMA if IOMMU_SUPPORT
> select IRQ_DOMAIN
> select IRQ_FORCED_THREADING
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index fbc366d..6d37fad 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -26,6 +26,11 @@
> #include <asm/irq.h>
> #include <asm/stack_pointer.h>
>
> +#define FAKE_FRAME(frame, my_func) do { \
> + frame.fp = (unsigned long)__builtin_frame_address(0); \
> + frame.pc = (unsigned long)my_func; \
> +} while (0)
> +
> /*
> * AArch64 PCS assigns the frame pointer to x29.
> *
> @@ -94,6 +99,77 @@ void notrace walk_stackframe(struct task_struct *tsk, struct stackframe *frame,
> }
> }
>
> +struct check_frame_arg {
> + unsigned long obj_start;
> + unsigned long obj_end;
> + unsigned long frame_start;
> + int discard_frames;
> + int err;
> +};
> +
> +static int check_frame(struct stackframe *frame, void *d)
> +{
> + struct check_frame_arg *arg = d;
> + unsigned long frame_end = frame->fp;
> +
> + /* object overlaps multiple frames */
> + if (arg->obj_start < frame->fp && frame->fp < arg->obj_end) {
> + arg->err = BAD_STACK;
> + return 1;
> + }
> +
> + /*
> + * Discard frames and check object is in a frame written early
> + * enough.
> + */
> + if (arg->discard_frames)
> + arg->discard_frames--;
> + else if ((arg->frame_start <= arg->obj_start &&
> + arg->obj_start < frame_end) &&
> + (arg->frame_start < arg->obj_end && arg->obj_end <= frame_end))
> + return 1;
> +
> + /* object exists in a previous frame */
> + if (arg->obj_end < arg->frame_start) {
> + arg->err = BAD_STACK;
> + return 1;
> + }
> +
> + arg->frame_start = frame_end + 0x10;
> +
> + return 0;
> +}
> +
> +/* Check obj doesn't overlap a stack frame record */
> +int arch_within_stack_frames(const void *stack,
> + const void *stack_end,
> + const void *obj, unsigned long obj_len)
> +{
> + struct stackframe frame;
> + struct check_frame_arg arg;
> +
> + if (!IS_ENABLED(CONFIG_FRAME_POINTER))
> + return NOT_STACK;
> +
> + arg.err = GOOD_FRAME;
> + arg.obj_start = (unsigned long)obj;
> + arg.obj_end = arg.obj_start + obj_len;
> +
> + FAKE_FRAME(frame, arch_within_stack_frames);
> + arg.frame_start = frame.fp;
> +
> + /*
> + * Skip 4 non-inlined frames: <fake frame>,
> + * arch_within_stack_frames(), check_stack_object() and
> + * __check_object_size().
> + */
> + arg.discard_frames = 4;
> +
> + walk_stackframe(current, &frame, check_frame, &arg);
> +
> + return arg.err;
> +}
> +
> #ifdef CONFIG_STACKTRACE
> struct stack_trace_data {
> struct stack_trace *trace;
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
[not found] ` <CAGXu5jJbPGifqTkKkqP3OJTCY=YO9+d_fRdvY-Fj_41HH4HFCw@mail.gmail.com>
@ 2018-04-04 23:11 ` Kees Cook
2018-04-09 5:40 ` Keun-O Park
0 siblings, 1 reply; 5+ messages in thread
From: Kees Cook @ 2018-04-04 23:11 UTC (permalink / raw)
To: Keun-O Park
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park, Sodagudi Prasad, Josh Poimboeuf,
Ingo Molnar, LKML
[resending with the CCs I forgot...]
On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.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@darkmatter.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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames
2018-04-04 22:55 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames Kees Cook
@ 2018-04-08 6:39 ` Keun-O Park
0 siblings, 0 replies; 5+ messages in thread
From: Keun-O Park @ 2018-04-08 6:39 UTC (permalink / raw)
To: Kees Cook
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park, Sodagudi Prasad, Josh Poimboeuf,
Ingo Molnar, LKML
Hi Kees,
On Thu, Apr 5, 2018 at 2:55 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
>> From: James Morse <james.morse@arm.com>
>>
>> This implements arch_within_stack_frames() for arm64 that should
>> validate if a given object is contained by a kernel stack frame.
>>
>> Signed-off-by: James Morse <james.morse@arm.com>
>> Reviewed-by: Sahara <keun-o.park@darkmatter.ae>
>
> Looks good to me. Does this end up passing the LKDTM
> USERCOPY_STACK_FRAME_TO and USERCOPY_STACK_FRAME_FROM tests?
Yes, this passes those two LKDTM tests.
Thanks.
BR
Sahara
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
2018-04-04 23:11 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder Kees Cook
@ 2018-04-09 5:40 ` Keun-O Park
0 siblings, 0 replies; 5+ messages in thread
From: Keun-O Park @ 2018-04-09 5:40 UTC (permalink / raw)
To: Kees Cook
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park, Sodagudi Prasad, Josh Poimboeuf,
Ingo Molnar, LKML
Hi Kees,
On Thu, Apr 5, 2018 at 3:11 AM, Kees Cook <keescook@chromium.org> wrote:
> [resending with the CCs I forgot...]
>
> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
>> From: Sahara <keun-o.park@darkmatter.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@darkmatter.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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-04-09 5:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1519899591-29761-1-git-send-email-kpark3469@gmail.com>
[not found] ` <1519899591-29761-2-git-send-email-kpark3469@gmail.com>
2018-04-04 22:52 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h Kees Cook
[not found] ` <1519899591-29761-3-git-send-email-kpark3469@gmail.com>
2018-04-04 22:55 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames Kees Cook
2018-04-08 6:39 ` Keun-O Park
[not found] ` <1519899591-29761-4-git-send-email-kpark3469@gmail.com>
[not found] ` <1519899591-29761-5-git-send-email-kpark3469@gmail.com>
[not found] ` <CAGXu5jJbPGifqTkKkqP3OJTCY=YO9+d_fRdvY-Fj_41HH4HFCw@mail.gmail.com>
2018-04-04 23:11 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder Kees Cook
2018-04-09 5:40 ` Keun-O Park
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).