linux-kernel - [PATCH v4 55/57] x86/mm: simplify starting frame logic for hardened usercopy

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

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

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

Date:	Thu, 18 Aug 2016 08:06:35 -0500
From:	Josh Poimboeuf <jpoimboe@...hat.com>
To:	Thomas Gleixner <tglx@...utronix.de>,
	Ingo Molnar <mingo@...nel.org>,
	"H . Peter Anvin" <hpa@...or.com>
Cc:	x86@...nel.org, linux-kernel@...r.kernel.org,
	Andy Lutomirski <luto@...capital.net>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Brian Gerst <brgerst@...il.com>,
	Kees Cook <keescook@...omium.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Frederic Weisbecker <fweisbec@...il.com>,
	Byungchul Park <byungchul.park@....com>,
	Nilay Vaish <nilayvaish@...il.com>
Subject: [PATCH v4 55/57] x86/mm: simplify starting frame logic for hardened usercopy

Currently, arch_within_stack_frames() has to manually find the stack
frame of its great-grandparent (i.e., it's caller's caller's caller).
This is somewhat fragile because it relies on the current call path and
inlining decisions.

Get the starting frame address closer to the source, in
check_stack_object(), and pass it to arch_within_stack_frames().

Signed-off-by: Josh Poimboeuf <jpoimboe@...hat.com>
---
 arch/x86/include/asm/thread_info.h |  2 ++
 arch/x86/lib/usercopy.c            | 12 ++++--------
 include/linux/thread_info.h        |  1 +
 mm/usercopy.c                      | 15 ++++++++++-----
 4 files changed, 17 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/thread_info.h b/arch/x86/include/asm/thread_info.h
index fd849e6..0f27e04 100644
--- a/arch/x86/include/asm/thread_info.h
+++ b/arch/x86/include/asm/thread_info.h
@@ -180,10 +180,12 @@ static inline unsigned long current_stack_pointer(void)
 #ifdef CONFIG_FRAME_POINTER
 int arch_within_stack_frames(const void * const stack,
 			     const void * const stackend,
+			     void *first_frame,
 			     const void *obj, unsigned long len);
 #else
 static inline int arch_within_stack_frames(const void * const stack,
 					   const void * const stackend,
+					   void *first_frame
 					   const void *obj, unsigned long len)
 {
 	return 0;
diff --git a/arch/x86/lib/usercopy.c b/arch/x86/lib/usercopy.c
index 8fe0a9c..fe0b233 100644
--- a/arch/x86/lib/usercopy.c
+++ b/arch/x86/lib/usercopy.c
@@ -48,20 +48,16 @@ EXPORT_SYMBOL_GPL(copy_from_user_nmi);
  */
 int arch_within_stack_frames(const void * const stack,
 			     const void * const stackend,
+			     void *first_frame,
 			     const void *obj, unsigned long len)
 {
 	struct unwind_state state;
 	const void *frame, *frame_end;
 
-	/*
-	 * Start at the end of our grandparent's frame (beginning of
-	 * great-grandparent's frame).
-	 */
-	unwind_start(&state, current, NULL, NULL);
-	if (WARN_ON_ONCE(!unwind_next_frame(&state) ||
-			 !unwind_next_frame(&state)))
-		return 0;
+	unwind_start(&state, current, NULL, first_frame);
 	frame = unwind_get_stack_ptr(&state);
+	if (WARN_ON_ONCE(frame != first_frame))
+		return 0;
 
 	/*
 	 * low ----------------------------------------------> high
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index cbd8990..aa58813 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -108,6 +108,7 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 #ifndef CONFIG_HAVE_ARCH_WITHIN_STACK_FRAMES
 static inline int arch_within_stack_frames(const void * const stack,
 					   const void * const stackend,
+					   void *first_frame,
 					   const void *obj, unsigned long len)
 {
 	return 0;
diff --git a/mm/usercopy.c b/mm/usercopy.c
index 8ebae91..359249c 100644
--- a/mm/usercopy.c
+++ b/mm/usercopy.c
@@ -26,8 +26,8 @@ enum {
 };
 
 /*
- * Checks if a given pointer and length is contained by the current
- * stack frame (if possible).
+ * Checks if a given pointer and length is contained by a frame in
+ * the current stack (if possible).
  *
  * Returns:
  *	NOT_STACK: not at all on the stack
@@ -35,7 +35,8 @@ enum {
  *	GOOD_STACK: fully on the stack (when can't do frame-checking)
  *	BAD_STACK: error condition (invalid stack position or bad stack frame)
  */
-static noinline int check_stack_object(const void *obj, unsigned long len)
+static __always_inline int check_stack_object(const void *obj,
+					      unsigned long len)
 {
 	const void * const stack = task_stack_page(current);
 	const void * const stackend = stack + THREAD_SIZE;
@@ -53,8 +54,12 @@ static noinline int check_stack_object(const void *obj, unsigned long len)
 	if (obj < stack || stackend < obj + len)
 		return BAD_STACK;
 
-	/* Check if object is safely within a valid frame. */
-	ret = arch_within_stack_frames(stack, stackend, obj, len);
+	/*
+	 * Starting with the caller's stack frame, check if the object
+	 * is safely within a valid frame.
+	 */
+	ret = arch_within_stack_frames(stack, stackend,
+				       __builtin_frame_address(0), obj, len);
 	if (ret)
 		return ret;
 
-- 
2.7.4