* [PATCH 0/4] usercopy: reimplement arch_within_stack_frames
@ 2018-03-01 10:19 kpark3469
2018-03-01 10:19 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h kpark3469
2018-03-26 14:35 ` [PATCH 0/4] usercopy: reimplement arch_within_stack_frames Will Deacon
0 siblings, 2 replies; 17+ messages in thread
From: kpark3469 @ 2018-03-01 10:19 UTC (permalink / raw)
To: kernel-hardening
Cc: keescook, james.morse, catalin.marinas, will.deacon,
mark.rutland, keun-o.park
From: Sahara <keun-o.park@darkmatter.ae>
This series of patches introduce the arm64 arch_within_stack_frames
implementation using stacktrace functions. Also the base code is
moved from thread_info.h to stacktrace.h. x86 code is reimplemented
to use frame pointer unwinder functions.
Note: The code is still missing in case of using x86 ORC unwinder and
guess unwinder.
James Morse (1):
arm64: usercopy: implement arch_within_stack_frames
Sahara (3):
stacktrace: move arch_within_stack_frames from thread_info.h
arm64: usercopy: consider dynamic array stack variable
x86: usercopy: reimplement arch_within_stack_frames with unwinder
arch/arm/kernel/stacktrace.c | 1 -
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/stacktrace.c | 82 +++++++++++++++++++++++++++++++++++++-
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/include/asm/unwind.h | 5 +++
arch/x86/kernel/Makefile | 2 +-
arch/x86/kernel/stacktrace.c | 81 ++++++++++++++++++++++++++++++++++++-
arch/x86/kernel/unwind_frame.c | 4 +-
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 +-
22 files changed, 196 insertions(+), 92 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h
2018-03-01 10:19 [PATCH 0/4] usercopy: reimplement arch_within_stack_frames kpark3469
@ 2018-03-01 10:19 ` kpark3469
2018-03-01 10:19 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames kpark3469
2018-04-04 22:52 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h Kees Cook
2018-03-26 14:35 ` [PATCH 0/4] usercopy: reimplement arch_within_stack_frames Will Deacon
1 sibling, 2 replies; 17+ messages in thread
From: kpark3469 @ 2018-03-01 10:19 UTC (permalink / raw)
To: kernel-hardening
Cc: keescook, james.morse, catalin.marinas, will.deacon,
mark.rutland, keun-o.park
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>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames
2018-03-01 10:19 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h kpark3469
@ 2018-03-01 10:19 ` kpark3469
2018-03-01 10:19 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable kpark3469
2018-04-04 22:55 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames Kees Cook
2018-04-04 22:52 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h Kees Cook
1 sibling, 2 replies; 17+ messages in thread
From: kpark3469 @ 2018-03-01 10:19 UTC (permalink / raw)
To: kernel-hardening
Cc: keescook, james.morse, catalin.marinas, will.deacon,
mark.rutland, keun-o.park
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>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable
2018-03-01 10:19 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames kpark3469
@ 2018-03-01 10:19 ` kpark3469
2018-03-01 10:19 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder kpark3469
2018-04-04 22:57 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable Kees Cook
2018-04-04 22:55 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames Kees Cook
1 sibling, 2 replies; 17+ messages in thread
From: kpark3469 @ 2018-03-01 10:19 UTC (permalink / raw)
To: kernel-hardening
Cc: keescook, james.morse, catalin.marinas, will.deacon,
mark.rutland, keun-o.park
From: Sahara <keun-o.park@darkmatter.ae>
When an array is dynamically declared, the array may be placed
at next frame. If this variable is used for usercopy, then it
will cause an Oops because the current check code does not allow
this exceptional case.
low -----------------------------------------------------> high
[__check_object_size fp][lr][args][local vars][caller_fp][lr]
^----------------^
dynamically allocated stack variable of
caller frame copies are allowed within here
< example code snippet >
array_size = get_random_int() & 0x0f;
if (to_user) {
unsigned char array[array_size];
if (copy_to_user((void __user *)user_addr, array,
unconst + sizeof(array))) {
Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
---
arch/arm64/kernel/stacktrace.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index 6d37fad..75a8f20 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -162,8 +162,13 @@ int arch_within_stack_frames(const void *stack,
* Skip 4 non-inlined frames: <fake frame>,
* arch_within_stack_frames(), check_stack_object() and
* __check_object_size().
+ *
+ * From Akashi's report, an object may be placed between next caller's
+ * frame, when the object is created as dynamic array.
+ * Setting the discard_frames to 3 is proper to catch this exceptional
+ * case.
*/
- arg.discard_frames = 4;
+ arg.discard_frames = 3;
walk_stackframe(current, &frame, check_frame, &arg);
--
2.7.4
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
2018-03-01 10:19 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable kpark3469
@ 2018-03-01 10:19 ` kpark3469
2018-04-04 23:10 ` Kees Cook
2018-04-04 22:57 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable Kees Cook
1 sibling, 1 reply; 17+ messages in thread
From: kpark3469 @ 2018-03-01 10:19 UTC (permalink / raw)
To: kernel-hardening
Cc: keescook, james.morse, catalin.marinas, will.deacon,
mark.rutland, keun-o.park
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>
---
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;
+}
+
+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;
+}
+
/*
* 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
+ * 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.
- */
- 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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] usercopy: reimplement arch_within_stack_frames
2018-03-01 10:19 [PATCH 0/4] usercopy: reimplement arch_within_stack_frames kpark3469
2018-03-01 10:19 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h kpark3469
@ 2018-03-26 14:35 ` Will Deacon
2018-03-28 10:16 ` Keun-O Park
1 sibling, 1 reply; 17+ messages in thread
From: Will Deacon @ 2018-03-26 14:35 UTC (permalink / raw)
To: kpark3469
Cc: kernel-hardening, keescook, james.morse, catalin.marinas,
mark.rutland, keun-o.park
Hi Sahara,
On Thu, Mar 01, 2018 at 02:19:47PM +0400, kpark3469@gmail.com wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> This series of patches introduce the arm64 arch_within_stack_frames
> implementation using stacktrace functions. Also the base code is
> moved from thread_info.h to stacktrace.h. x86 code is reimplemented
> to use frame pointer unwinder functions.
What's the status on this series? I'm ok taking the arm64 patch from James
to bring us up to speed with x86, but it doesn't make sense without the
first patch in the series because the arch callback is out-of-line.
Will
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] usercopy: reimplement arch_within_stack_frames
2018-03-26 14:35 ` [PATCH 0/4] usercopy: reimplement arch_within_stack_frames Will Deacon
@ 2018-03-28 10:16 ` Keun-O Park
2018-04-04 22:46 ` Kees Cook
0 siblings, 1 reply; 17+ messages in thread
From: Keun-O Park @ 2018-03-28 10:16 UTC (permalink / raw)
To: Will Deacon
Cc: kernel-hardening, Kees Cook, James Morse, Catalin Marinas,
Mark Rutland, keun-o.park
Hi Will,
On Mon, Mar 26, 2018 at 4:35 PM, Will Deacon <will.deacon@arm.com> wrote:
> Hi Sahara,
>
> On Thu, Mar 01, 2018 at 02:19:47PM +0400, kpark3469@gmail.com wrote:
>> From: Sahara <keun-o.park@darkmatter.ae>
>>
>> This series of patches introduce the arm64 arch_within_stack_frames
>> implementation using stacktrace functions. Also the base code is
>> moved from thread_info.h to stacktrace.h. x86 code is reimplemented
>> to use frame pointer unwinder functions.
>
> What's the status on this series? I'm ok taking the arm64 patch from James
> to bring us up to speed with x86, but it doesn't make sense without the
> first patch in the series because the arch callback is out-of-line.
>
> Will
I haven't received any response from Kees or other engineers about
this series of patches yet.
Hi Kees, any comment on these patches to put these on the next stage?
Thanks.
BR
Sahara
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/4] usercopy: reimplement arch_within_stack_frames
2018-03-28 10:16 ` Keun-O Park
@ 2018-04-04 22:46 ` Kees Cook
0 siblings, 0 replies; 17+ messages in thread
From: Kees Cook @ 2018-04-04 22:46 UTC (permalink / raw)
To: Keun-O Park
Cc: Will Deacon, Kernel Hardening, James Morse, Catalin Marinas,
Mark Rutland, keun-o.park
On Wed, Mar 28, 2018 at 3:16 AM, Keun-O Park <kpark3469@gmail.com> wrote:
> Hi Will,
>
> On Mon, Mar 26, 2018 at 4:35 PM, Will Deacon <will.deacon@arm.com> wrote:
>> Hi Sahara,
>>
>> On Thu, Mar 01, 2018 at 02:19:47PM +0400, kpark3469@gmail.com wrote:
>>> From: Sahara <keun-o.park@darkmatter.ae>
>>>
>>> This series of patches introduce the arm64 arch_within_stack_frames
>>> implementation using stacktrace functions. Also the base code is
>>> moved from thread_info.h to stacktrace.h. x86 code is reimplemented
>>> to use frame pointer unwinder functions.
>>
>> What's the status on this series? I'm ok taking the arm64 patch from James
>> to bring us up to speed with x86, but it doesn't make sense without the
>> first patch in the series because the arch callback is out-of-line.
>>
>> Will
>
> I haven't received any response from Kees or other engineers about
> this series of patches yet.
> Hi Kees, any comment on these patches to put these on the next stage?
Hi! Sorry for the delay, I'll send some notes now...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h
2018-03-01 10:19 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h kpark3469
2018-03-01 10:19 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames kpark3469
@ 2018-04-04 22:52 ` Kees Cook
1 sibling, 0 replies; 17+ 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] 17+ messages in thread
* Re: [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames
2018-03-01 10:19 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames kpark3469
2018-03-01 10:19 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable kpark3469
@ 2018-04-04 22:55 ` Kees Cook
2018-04-08 6:39 ` Keun-O Park
1 sibling, 1 reply; 17+ 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] 17+ messages in thread
* Re: [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable
2018-03-01 10:19 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable kpark3469
2018-03-01 10:19 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder kpark3469
@ 2018-04-04 22:57 ` Kees Cook
2018-04-08 6:45 ` Keun-O Park
1 sibling, 1 reply; 17+ messages in thread
From: Kees Cook @ 2018-04-04 22:57 UTC (permalink / raw)
To: Keun-O Park
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park
On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
> From: Sahara <keun-o.park@darkmatter.ae>
>
> When an array is dynamically declared, the array may be placed
> at next frame. If this variable is used for usercopy, then it
> will cause an Oops because the current check code does not allow
> this exceptional case.
>
> low -----------------------------------------------------> high
> [__check_object_size fp][lr][args][local vars][caller_fp][lr]
> ^----------------^
> dynamically allocated stack variable of
> caller frame copies are allowed within here
>
> < example code snippet >
> array_size = get_random_int() & 0x0f;
> if (to_user) {
> unsigned char array[array_size];
> if (copy_to_user((void __user *)user_addr, array,
> unconst + sizeof(array))) {
And once we have -Wvla in the build[1] by default we can revert this
and ignore the VLA case, yes?
Reviewed-by: Kees Cook <keescook@chromium.org>
-Kees
[1] https://lkml.org/lkml/2018/3/7/621
>
> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
> ---
> arch/arm64/kernel/stacktrace.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
> index 6d37fad..75a8f20 100644
> --- a/arch/arm64/kernel/stacktrace.c
> +++ b/arch/arm64/kernel/stacktrace.c
> @@ -162,8 +162,13 @@ int arch_within_stack_frames(const void *stack,
> * Skip 4 non-inlined frames: <fake frame>,
> * arch_within_stack_frames(), check_stack_object() and
> * __check_object_size().
> + *
> + * From Akashi's report, an object may be placed between next caller's
> + * frame, when the object is created as dynamic array.
> + * Setting the discard_frames to 3 is proper to catch this exceptional
> + * case.
> */
> - arg.discard_frames = 4;
> + arg.discard_frames = 3;
>
> walk_stackframe(current, &frame, check_frame, &arg);
>
> --
> 2.7.4
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
2018-03-01 10:19 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder kpark3469
@ 2018-04-04 23:10 ` Kees Cook
2018-04-04 23:11 ` Kees Cook
0 siblings, 1 reply; 17+ messages in thread
From: Kees Cook @ 2018-04-04 23:10 UTC (permalink / raw)
To: Keun-O Park
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
2018-04-04 23:10 ` Kees Cook
@ 2018-04-04 23:11 ` Kees Cook
2018-04-09 5:40 ` Keun-O Park
0 siblings, 1 reply; 17+ 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] 17+ 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; 17+ 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] 17+ messages in thread
* Re: [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable
2018-04-04 22:57 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable Kees Cook
@ 2018-04-08 6:45 ` Keun-O Park
2018-04-09 5:54 ` Keun-O Park
0 siblings, 1 reply; 17+ messages in thread
From: Keun-O Park @ 2018-04-08 6:45 UTC (permalink / raw)
To: Kees Cook
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park
On Thu, Apr 5, 2018 at 2:57 AM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
>> From: Sahara <keun-o.park@darkmatter.ae>
>>
>> When an array is dynamically declared, the array may be placed
>> at next frame. If this variable is used for usercopy, then it
>> will cause an Oops because the current check code does not allow
>> this exceptional case.
>>
>> low -----------------------------------------------------> high
>> [__check_object_size fp][lr][args][local vars][caller_fp][lr]
>> ^----------------^
>> dynamically allocated stack variable of
>> caller frame copies are allowed within here
>>
>> < example code snippet >
>> array_size = get_random_int() & 0x0f;
>> if (to_user) {
>> unsigned char array[array_size];
>> if (copy_to_user((void __user *)user_addr, array,
>> unconst + sizeof(array))) {
>
> And once we have -Wvla in the build[1] by default we can revert this
> and ignore the VLA case, yes?
Yes, you are right. Once we enable -Wvla, then we don't need this.
Thanks.
BR,
Sahara
>
> Reviewed-by: Kees Cook <keescook@chromium.org>
>
> -Kees
>
> [1] https://lkml.org/lkml/2018/3/7/621
>
>>
>> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
>> ---
>> arch/arm64/kernel/stacktrace.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>> index 6d37fad..75a8f20 100644
>> --- a/arch/arm64/kernel/stacktrace.c
>> +++ b/arch/arm64/kernel/stacktrace.c
>> @@ -162,8 +162,13 @@ int arch_within_stack_frames(const void *stack,
>> * Skip 4 non-inlined frames: <fake frame>,
>> * arch_within_stack_frames(), check_stack_object() and
>> * __check_object_size().
>> + *
>> + * From Akashi's report, an object may be placed between next caller's
>> + * frame, when the object is created as dynamic array.
>> + * Setting the discard_frames to 3 is proper to catch this exceptional
>> + * case.
>> */
>> - arg.discard_frames = 4;
>> + arg.discard_frames = 3;
>>
>> walk_stackframe(current, &frame, check_frame, &arg);
>>
>> --
>> 2.7.4
>>
>
>
>
> --
> Kees Cook
> Pixel Security
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder
2018-04-04 23:11 ` Kees Cook
@ 2018-04-09 5:40 ` Keun-O Park
0 siblings, 0 replies; 17+ 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] 17+ messages in thread
* Re: [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable
2018-04-08 6:45 ` Keun-O Park
@ 2018-04-09 5:54 ` Keun-O Park
0 siblings, 0 replies; 17+ messages in thread
From: Keun-O Park @ 2018-04-09 5:54 UTC (permalink / raw)
To: Kees Cook
Cc: Kernel Hardening, James Morse, Catalin Marinas, Will Deacon,
Mark Rutland, keun-o.park
I removed this patch from v2 patchset.
I noticed that this patch can not pass the current lkdtm
stack_frame_to/from tests.
Because the bad_stack is placed before the do_usercopy_stack frame,
this patch makes bad_stack pass the check.
It's not what we want to happen.
For now and later, I think this might be the best solution.
These are the each frame's information in walk_stackframe, when
discard_frame is set to 3 in order to filter the variable length
array.
root@genericarmv8:/sys/kernel/debug/provoke-crash# echo
USERCOPY_STACK_FRAME_TO > DIRECT
[ 3726.088794] lkdtm: Performing direct entry USERCOPY_STACK_FRAME_TO
[ 3726.088867] lkdtm: >>> buf=0xffff00000bf7bc18
[ 3726.088936] lkdtm: >>> do_usercopy_stack_callee:
fp=ffff00000bf7bbf0 xfp=ffff00000bf7bc40
[ 3726.089037] lkdtm: >>> do_usercopy_stack:
good_stack=0xffff00000bf7bc98, bad_stack=0xffff00000bf7bc18
[ 3726.089129] lkdtm: >>> do_usercopy_stack: fp=ffff00000bf7bc40
xfp=ffff00000bf7bcc0
[ 3726.089203] lkdtm: attempting good copy_to_user of local stack
[ 3726.089267] =====================================
[ 3726.089358] > PC = arch_within_stack_frames+0x0/0x88
[ 3726.089423] > discard_f = 3
[ 3726.089477] > frm_start = 0xffff00000bf7bb90
[ 3726.089539] > frame_end = 0xffff00000bf7bb90
[ 3726.089601] > obj start = 0xffff00000bf7bc98
[ 3726.089662] > obj end = 0xffff00000bf7bcb8
[ 3726.089720] =====================================
[ 3726.089803] > PC = check_stack_object+0x44/0x68
[ 3726.089867] > discard_f = 2
[ 3726.089921] > frm_start = 0xffff00000bf7bba0
[ 3726.089982] > frame_end = 0xffff00000bf7bbf0
[ 3726.090044] > obj start = 0xffff00000bf7bc98
[ 3726.090105] > obj end = 0xffff00000bf7bcb8
[ 3726.090163] =====================================
[ 3726.090247] > PC = __check_object_size+0x50/0x1e0
[ 3726.090311] > discard_f = 1
[ 3726.090365] > frm_start = 0xffff00000bf7bc00
[ 3726.090427] > frame_end = 0xffff00000bf7bc00
[ 3726.090489] > obj start = 0xffff00000bf7bc98
[ 3726.090550] > obj end = 0xffff00000bf7bcb8
[ 3726.090608] =====================================
[ 3726.090688] > PC = do_usercopy_stack+0x188/0x3c0
[ 3726.090752] > discard_f = 0
[ 3726.090806] > frm_start = 0xffff00000bf7bc10
[ 3726.090867] > frame_end = 0xffff00000bf7bc40
[ 3726.091025] > obj start = 0xffff00000bf7bc98
[ 3726.091134] > obj end = 0xffff00000bf7bcb8
[ 3726.091240] =====================================
[ 3726.091365] > PC = lkdtm_USERCOPY_STACK_FRAME_TO+0x24/0x38
[ 3726.091495] > discard_f = 0
[ 3726.091596] > frm_start = 0xffff00000bf7bc50
[ 3726.091702] > frame_end = 0xffff00000bf7bcc0
[ 3726.091806] > obj start = 0xffff00000bf7bc98
[ 3726.091909] > obj end = 0xffff00000bf7bcb8
[ 3726.092037] lkdtm: attempting bad copy_to_user of distant stack
[ 3726.092163] =====================================
[ 3726.092298] > PC = arch_within_stack_frames+0x0/0x88
[ 3726.092424] > discard_f = 3
[ 3726.092528] > frm_start = 0xffff00000bf7bb90
[ 3726.092634] > frame_end = 0xffff00000bf7bb90
[ 3726.092696] > obj start = 0xffff00000bf7bc18
[ 3726.092757] > obj end = 0xffff00000bf7bc38
[ 3726.092815] =====================================
[ 3726.092897] > PC = check_stack_object+0x44/0x68
[ 3726.092960] > discard_f = 2
[ 3726.093014] > frm_start = 0xffff00000bf7bba0
[ 3726.093075] > frame_end = 0xffff00000bf7bbf0
[ 3726.093136] > obj start = 0xffff00000bf7bc18
[ 3726.093198] > obj end = 0xffff00000bf7bc38
[ 3726.093255] =====================================
[ 3726.093338] > PC = __check_object_size+0x50/0x1e0
[ 3726.093402] > discard_f = 1
[ 3726.093456] > frm_start = 0xffff00000bf7bc00
[ 3726.093517] > frame_end = 0xffff00000bf7bc00
[ 3726.093578] > obj start = 0xffff00000bf7bc18
[ 3726.093640] > obj end = 0xffff00000bf7bc38
[ 3726.093697] =====================================
[ 3726.093777] > PC = do_usercopy_stack+0x318/0x3c0
[ 3726.093840] > discard_f = 0
[ 3726.093894] > frm_start = 0xffff00000bf7bc10
[ 3726.093955] > frame_end = 0xffff00000bf7bc40
[ 3726.094016] > obj start = 0xffff00000bf7bc18
[ 3726.094077] > obj end = 0xffff00000bf7bc38
Thanks.
BR
Sahara
On Sun, Apr 8, 2018 at 10:45 AM, Keun-O Park <kpark3469@gmail.com> wrote:
> On Thu, Apr 5, 2018 at 2:57 AM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Mar 1, 2018 at 2:19 AM, <kpark3469@gmail.com> wrote:
>>> From: Sahara <keun-o.park@darkmatter.ae>
>>>
>>> When an array is dynamically declared, the array may be placed
>>> at next frame. If this variable is used for usercopy, then it
>>> will cause an Oops because the current check code does not allow
>>> this exceptional case.
>>>
>>> low -----------------------------------------------------> high
>>> [__check_object_size fp][lr][args][local vars][caller_fp][lr]
>>> ^----------------^
>>> dynamically allocated stack variable of
>>> caller frame copies are allowed within here
>>>
>>> < example code snippet >
>>> array_size = get_random_int() & 0x0f;
>>> if (to_user) {
>>> unsigned char array[array_size];
>>> if (copy_to_user((void __user *)user_addr, array,
>>> unconst + sizeof(array))) {
>>
>> And once we have -Wvla in the build[1] by default we can revert this
>> and ignore the VLA case, yes?
>
> Yes, you are right. Once we enable -Wvla, then we don't need this.
> Thanks.
>
> BR,
> Sahara
>
>
>>
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>>
>> -Kees
>>
>> [1] https://lkml.org/lkml/2018/3/7/621
>>
>>>
>>> Signed-off-by: Sahara <keun-o.park@darkmatter.ae>
>>> ---
>>> arch/arm64/kernel/stacktrace.c | 7 ++++++-
>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
>>> index 6d37fad..75a8f20 100644
>>> --- a/arch/arm64/kernel/stacktrace.c
>>> +++ b/arch/arm64/kernel/stacktrace.c
>>> @@ -162,8 +162,13 @@ int arch_within_stack_frames(const void *stack,
>>> * Skip 4 non-inlined frames: <fake frame>,
>>> * arch_within_stack_frames(), check_stack_object() and
>>> * __check_object_size().
>>> + *
>>> + * From Akashi's report, an object may be placed between next caller's
>>> + * frame, when the object is created as dynamic array.
>>> + * Setting the discard_frames to 3 is proper to catch this exceptional
>>> + * case.
>>> */
>>> - arg.discard_frames = 4;
>>> + arg.discard_frames = 3;
>>>
>>> walk_stackframe(current, &frame, check_frame, &arg);
>>>
>>> --
>>> 2.7.4
>>>
>>
>>
>>
>> --
>> Kees Cook
>> Pixel Security
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-04-09 5:54 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 10:19 [PATCH 0/4] usercopy: reimplement arch_within_stack_frames kpark3469
2018-03-01 10:19 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h kpark3469
2018-03-01 10:19 ` [PATCH 2/4] arm64: usercopy: implement arch_within_stack_frames kpark3469
2018-03-01 10:19 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable kpark3469
2018-03-01 10:19 ` [PATCH 4/4] x86: usercopy: reimplement arch_within_stack_frames with unwinder kpark3469
2018-04-04 23:10 ` Kees Cook
2018-04-04 23:11 ` Kees Cook
2018-04-09 5:40 ` Keun-O Park
2018-04-04 22:57 ` [PATCH 3/4] arm64: usercopy: consider dynamic array stack variable Kees Cook
2018-04-08 6:45 ` Keun-O Park
2018-04-09 5:54 ` Keun-O Park
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
2018-04-04 22:52 ` [PATCH 1/4] stacktrace: move arch_within_stack_frames from thread_info.h Kees Cook
2018-03-26 14:35 ` [PATCH 0/4] usercopy: reimplement arch_within_stack_frames Will Deacon
2018-03-28 10:16 ` Keun-O Park
2018-04-04 22:46 ` Kees Cook
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.