All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] x86/unwind: add some unwinder warnings
@ 2016-10-26 15:41 Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 1/4] x86/unwind: warn on bad frame pointer Josh Poimboeuf
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-26 15:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

Detect some stack error conditions in the unwinder and warn about them.

These warnings may help with debugging stack corruption issues.

Josh Poimboeuf (4):
  x86/unwind: warn on bad frame pointer
  x86/unwind: ensure stack grows down
  x86/dumpstack: warn on stack recursion
  x86/unwind: detect bad stack return address

 arch/x86/kernel/dumpstack_32.c |  5 +++-
 arch/x86/kernel/dumpstack_64.c |  5 +++-
 arch/x86/kernel/unwind_frame.c | 56 +++++++++++++++++++++++++++++++++++++++---
 3 files changed, 61 insertions(+), 5 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH 1/4] x86/unwind: warn on bad frame pointer
  2016-10-26 15:41 [PATCH 0/4] x86/unwind: add some unwinder warnings Josh Poimboeuf
@ 2016-10-26 15:41 ` Josh Poimboeuf
  2016-10-27  7:37   ` [tip:x86/asm] x86/unwind: Warn " tip-bot for Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 2/4] x86/unwind: ensure stack grows down Josh Poimboeuf
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-26 15:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

Detect situations in the unwinder where the frame pointer refers to a
bad address, and print an appropriate warning.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/unwind_frame.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 5795427..9be9a8f 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -123,8 +123,17 @@ bool unwind_next_frame(struct unwind_state *state)
 	}
 
 	/* make sure the next frame's data is accessible */
-	if (!update_stack_state(state, next_frame, next_len))
-		return false;
+	if (!update_stack_state(state, next_frame, next_len)) {
+		/*
+		 * Don't warn on bad regs->bp.  An interrupt in entry code
+		 * might cause a false positive warning.
+		 */
+		if (state->regs)
+			goto the_end;
+
+		goto bad_address;
+	}
+
 	/* move to the next frame */
 	if (regs) {
 		state->regs = regs;
@@ -136,6 +145,11 @@ bool unwind_next_frame(struct unwind_state *state)
 
 	return true;
 
+bad_address:
+	printk_deferred_once(KERN_WARNING
+		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
+		state->bp, state->task->comm,
+		state->task->pid, next_bp);
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 2/4] x86/unwind: ensure stack grows down
  2016-10-26 15:41 [PATCH 0/4] x86/unwind: add some unwinder warnings Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 1/4] x86/unwind: warn on bad frame pointer Josh Poimboeuf
@ 2016-10-26 15:41 ` Josh Poimboeuf
  2016-10-27  6:30   ` Ingo Molnar
  2016-10-26 15:41 ` [PATCH 3/4] x86/dumpstack: warn on stack recursion Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 4/4] x86/unwind: detect bad stack return address Josh Poimboeuf
  3 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-26 15:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

Add a sanity check to ensure the stack only grows down, and print a
warning if the check fails.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/unwind_frame.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 9be9a8f..0d8d2f2 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -24,6 +24,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
+static 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))
+		return sizeof(*regs) - (2*sizeof(long));
+
+	return sizeof(*regs);
+}
+
 static bool is_last_task_frame(struct unwind_state *state)
 {
 	unsigned long bp = (unsigned long)state->bp;
@@ -71,6 +80,7 @@ bool unwind_next_frame(struct unwind_state *state)
 	struct pt_regs *regs;
 	unsigned long *next_bp, *next_frame;
 	size_t next_len;
+	enum stack_type prev_type = state->stack_info.type;
 
 	if (unwind_done(state))
 		return false;
@@ -134,6 +144,18 @@ bool unwind_next_frame(struct unwind_state *state)
 		goto bad_address;
 	}
 
+	/* make sure it only unwinds up and doesn't overlap the last frame */
+	if (state->stack_info.type == prev_type) {
+		if (state->regs &&
+		    (void *)next_frame < (void *)state->regs +
+					 regs_size(state->regs))
+			goto bad_address;
+
+		if (state->bp &&
+		    (void *)next_frame < (void *)state->bp + FRAME_HEADER_SIZE)
+			goto bad_address;
+	}
+
 	/* move to the next frame */
 	if (regs) {
 		state->regs = regs;
@@ -146,10 +168,16 @@ bool unwind_next_frame(struct unwind_state *state)
 	return true;
 
 bad_address:
-	printk_deferred_once(KERN_WARNING
-		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
-		state->bp, state->task->comm,
-		state->task->pid, next_bp);
+	if (state->regs)
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
+			state->regs, state->task->comm,
+			state->task->pid, next_frame);
+	else
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
+			state->bp, state->task->comm,
+			state->task->pid, next_frame);
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 3/4] x86/dumpstack: warn on stack recursion
  2016-10-26 15:41 [PATCH 0/4] x86/unwind: add some unwinder warnings Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 1/4] x86/unwind: warn on bad frame pointer Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 2/4] x86/unwind: ensure stack grows down Josh Poimboeuf
@ 2016-10-26 15:41 ` Josh Poimboeuf
  2016-10-27  7:38   ` [tip:x86/asm] x86/dumpstack: Warn " tip-bot for Josh Poimboeuf
  2016-10-26 15:41 ` [PATCH 4/4] x86/unwind: detect bad stack return address Josh Poimboeuf
  3 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-26 15:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

Print a warning if stack recursion is detected.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/dumpstack_32.c | 5 ++++-
 arch/x86/kernel/dumpstack_64.c | 5 ++++-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 90cf460..99ac8d2 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -109,8 +109,11 @@ int get_stack_info(unsigned long *stack, struct task_struct *task,
 	 * just break out and report an unknown stack type.
 	 */
 	if (visit_mask) {
-		if (*visit_mask & (1UL << info->type))
+		if (*visit_mask & (1UL << info->type)) {
+			printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n",
+					     info->type);
 			goto unknown;
+		}
 		*visit_mask |= 1UL << info->type;
 	}
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 310abf4..26e6e83 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -128,8 +128,11 @@ int get_stack_info(unsigned long *stack, struct task_struct *task,
 	 * just break out and report an unknown stack type.
 	 */
 	if (visit_mask) {
-		if (*visit_mask & (1UL << info->type))
+		if (*visit_mask & (1UL << info->type)) {
+			printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n",
+					     info->type);
 			goto unknown;
+		}
 		*visit_mask |= 1UL << info->type;
 	}
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH 4/4] x86/unwind: detect bad stack return address
  2016-10-26 15:41 [PATCH 0/4] x86/unwind: add some unwinder warnings Josh Poimboeuf
                   ` (2 preceding siblings ...)
  2016-10-26 15:41 ` [PATCH 3/4] x86/dumpstack: warn on stack recursion Josh Poimboeuf
@ 2016-10-26 15:41 ` Josh Poimboeuf
  2016-10-27  7:38   ` [tip:x86/asm] x86/unwind: Detect " tip-bot for Josh Poimboeuf
  3 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-26 15:41 UTC (permalink / raw)
  To: x86; +Cc: linux-kernel

If __kernel_text_address() doesn't recognize a return address on the
stack, it probably means that it's some generated code which
__kernel_text_address() doesn't know about yet.

Otherwise there's probably some stack corruption.

Either way, warn about it.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/unwind_frame.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 0d8d2f2..b6fcf44 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -20,7 +20,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 	addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
 				     addr_p);
 
-	return __kernel_text_address(addr) ? addr : 0;
+	if (!__kernel_text_address(addr)) {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: unrecognized kernel stack return address %p at %p in %s:%d\n",
+			(void *)addr, addr_p, state->task->comm,
+			state->task->pid);
+		return 0;
+	}
+
+	return addr;
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH 2/4] x86/unwind: ensure stack grows down
  2016-10-26 15:41 ` [PATCH 2/4] x86/unwind: ensure stack grows down Josh Poimboeuf
@ 2016-10-27  6:30   ` Ingo Molnar
  2016-10-27 13:10     ` [PATCH v2] " Josh Poimboeuf
  0 siblings, 1 reply; 11+ messages in thread
From: Ingo Molnar @ 2016-10-27  6:30 UTC (permalink / raw)
  To: Josh Poimboeuf; +Cc: x86, linux-kernel


* Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Add a sanity check to ensure the stack only grows down, and print a
> warning if the check fails.
> 
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/unwind_frame.c | 36 ++++++++++++++++++++++++++++++++----
>  1 file changed, 32 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
> index 9be9a8f..0d8d2f2 100644
> --- a/arch/x86/kernel/unwind_frame.c
> +++ b/arch/x86/kernel/unwind_frame.c
> @@ -24,6 +24,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
>  }
>  EXPORT_SYMBOL_GPL(unwind_get_return_address);
>  
> +static 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))
> +		return sizeof(*regs) - (2*sizeof(long));

Unnecessary parentheses.

> +
> +	return sizeof(*regs);
> +}
> +
>  static bool is_last_task_frame(struct unwind_state *state)
>  {
>  	unsigned long bp = (unsigned long)state->bp;
> @@ -71,6 +80,7 @@ bool unwind_next_frame(struct unwind_state *state)
>  	struct pt_regs *regs;
>  	unsigned long *next_bp, *next_frame;
>  	size_t next_len;
> +	enum stack_type prev_type = state->stack_info.type;
>  
>  	if (unwind_done(state))
>  		return false;
> @@ -134,6 +144,18 @@ bool unwind_next_frame(struct unwind_state *state)
>  		goto bad_address;
>  	}
>  
> +	/* make sure it only unwinds up and doesn't overlap the last frame */

Please capitalize comments - and if the comment refers to a control block please 
also add ':', i.e. something like:

	/* Make sure it only unwinds up and doesn't overlap the last frame: */

This also applies to other places in this patch series.

> +	if (state->stack_info.type == prev_type) {
> +		if (state->regs &&
> +		    (void *)next_frame < (void *)state->regs +
> +					 regs_size(state->regs))

No line breaks that make the code worse please.

> +			goto bad_address;
> +
> +		if (state->bp &&
> +		    (void *)next_frame < (void *)state->bp + FRAME_HEADER_SIZE)

Ditto.

> +			goto bad_address;
> +	}
> +
>  	/* move to the next frame */
>  	if (regs) {
>  		state->regs = regs;
> @@ -146,10 +168,16 @@ bool unwind_next_frame(struct unwind_state *state)
>  	return true;
>  
>  bad_address:
> -	printk_deferred_once(KERN_WARNING
> -		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
> -		state->bp, state->task->comm,
> -		state->task->pid, next_bp);
> +	if (state->regs)
> +		printk_deferred_once(KERN_WARNING
> +			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
> +			state->regs, state->task->comm,
> +			state->task->pid, next_frame);
> +	else
> +		printk_deferred_once(KERN_WARNING
> +			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
> +			state->bp, state->task->comm,
> +			state->task->pid, next_frame);

Multi-line statements should have curly braces.

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [tip:x86/asm] x86/unwind: Warn on bad frame pointer
  2016-10-26 15:41 ` [PATCH 1/4] x86/unwind: warn on bad frame pointer Josh Poimboeuf
@ 2016-10-27  7:37   ` tip-bot for Josh Poimboeuf
  0 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2016-10-27  7:37 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: torvalds, jpoimboe, tglx, dvlasenk, luto, peterz, linux-kernel,
	brgerst, mingo, hpa, bp

Commit-ID:  c32c47c68a0ae701088c5b2c3798856ed16746ae
Gitweb:     http://git.kernel.org/tip/c32c47c68a0ae701088c5b2c3798856ed16746ae
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 26 Oct 2016 10:41:48 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 27 Oct 2016 08:32:37 +0200

x86/unwind: Warn on bad frame pointer

Detect situations in the unwinder where the frame pointer refers to a
bad address, and print an appropriate warning.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/03c888f6f7414d54fa56b393ea25482be6899b5f.1477496147.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/unwind_frame.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 5795427..9be9a8f 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -123,8 +123,17 @@ bool unwind_next_frame(struct unwind_state *state)
 	}
 
 	/* make sure the next frame's data is accessible */
-	if (!update_stack_state(state, next_frame, next_len))
-		return false;
+	if (!update_stack_state(state, next_frame, next_len)) {
+		/*
+		 * Don't warn on bad regs->bp.  An interrupt in entry code
+		 * might cause a false positive warning.
+		 */
+		if (state->regs)
+			goto the_end;
+
+		goto bad_address;
+	}
+
 	/* move to the next frame */
 	if (regs) {
 		state->regs = regs;
@@ -136,6 +145,11 @@ bool unwind_next_frame(struct unwind_state *state)
 
 	return true;
 
+bad_address:
+	printk_deferred_once(KERN_WARNING
+		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
+		state->bp, state->task->comm,
+		state->task->pid, next_bp);
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [tip:x86/asm] x86/dumpstack: Warn on stack recursion
  2016-10-26 15:41 ` [PATCH 3/4] x86/dumpstack: warn on stack recursion Josh Poimboeuf
@ 2016-10-27  7:38   ` tip-bot for Josh Poimboeuf
  0 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2016-10-27  7:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: torvalds, bp, tglx, mingo, luto, peterz, jpoimboe, linux-kernel,
	brgerst, dvlasenk, hpa

Commit-ID:  0d2b8579add41e08aa1110da864f1071d58e6048
Gitweb:     http://git.kernel.org/tip/0d2b8579add41e08aa1110da864f1071d58e6048
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 26 Oct 2016 10:41:50 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 27 Oct 2016 08:32:38 +0200

x86/dumpstack: Warn on stack recursion

Print a warning if stack recursion is detected.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/def18247aafaab480844484398e793f552b79bda.1477496147.git.jpoimboe@redhat.com
[ Unbroke the lines. ]
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/dumpstack_32.c | 4 +++-
 arch/x86/kernel/dumpstack_64.c | 4 +++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/dumpstack_32.c b/arch/x86/kernel/dumpstack_32.c
index 90cf460..d7d20999 100644
--- a/arch/x86/kernel/dumpstack_32.c
+++ b/arch/x86/kernel/dumpstack_32.c
@@ -109,8 +109,10 @@ recursion_check:
 	 * just break out and report an unknown stack type.
 	 */
 	if (visit_mask) {
-		if (*visit_mask & (1UL << info->type))
+		if (*visit_mask & (1UL << info->type)) {
+			printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n", info->type);
 			goto unknown;
+		}
 		*visit_mask |= 1UL << info->type;
 	}
 
diff --git a/arch/x86/kernel/dumpstack_64.c b/arch/x86/kernel/dumpstack_64.c
index 310abf4..ab0f8b9 100644
--- a/arch/x86/kernel/dumpstack_64.c
+++ b/arch/x86/kernel/dumpstack_64.c
@@ -128,8 +128,10 @@ recursion_check:
 	 * just break out and report an unknown stack type.
 	 */
 	if (visit_mask) {
-		if (*visit_mask & (1UL << info->type))
+		if (*visit_mask & (1UL << info->type)) {
+			printk_deferred_once(KERN_WARNING "WARNING: stack recursion on stack type %d\n", info->type);
 			goto unknown;
+		}
 		*visit_mask |= 1UL << info->type;
 	}
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [tip:x86/asm] x86/unwind: Detect bad stack return address
  2016-10-26 15:41 ` [PATCH 4/4] x86/unwind: detect bad stack return address Josh Poimboeuf
@ 2016-10-27  7:38   ` tip-bot for Josh Poimboeuf
  0 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2016-10-27  7:38 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: torvalds, luto, hpa, brgerst, linux-kernel, peterz, bp, dvlasenk,
	tglx, mingo, jpoimboe

Commit-ID:  b6959a362177053c1c90db6dc1af25b6bddd8548
Gitweb:     http://git.kernel.org/tip/b6959a362177053c1c90db6dc1af25b6bddd8548
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Wed, 26 Oct 2016 10:41:51 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 27 Oct 2016 08:32:38 +0200

x86/unwind: Detect bad stack return address

If __kernel_text_address() doesn't recognize a return address on the
stack, it probably means that it's some generated code which
__kernel_text_address() doesn't know about yet.

Otherwise there's probably some stack corruption.

Either way, warn about it.

Use printk_deferred_once() because the unwinder can be called with the
console lock by lockdep via save_stack_trace().

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/2d897898f324e275943b590d160b55e482bba65f.1477496147.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/unwind_frame.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 9be9a8f..5639db6 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -20,7 +20,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 	addr = ftrace_graph_ret_addr(state->task, &state->graph_idx, *addr_p,
 				     addr_p);
 
-	return __kernel_text_address(addr) ? addr : 0;
+	if (!__kernel_text_address(addr)) {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: unrecognized kernel stack return address %p at %p in %s:%d\n",
+			(void *)addr, addr_p, state->task->comm,
+			state->task->pid);
+		return 0;
+	}
+
+	return addr;
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH v2] x86/unwind: ensure stack grows down
  2016-10-27  6:30   ` Ingo Molnar
@ 2016-10-27 13:10     ` Josh Poimboeuf
  2016-10-28  6:49       ` [tip:x86/asm] x86/unwind: Ensure " tip-bot for Josh Poimboeuf
  0 siblings, 1 reply; 11+ messages in thread
From: Josh Poimboeuf @ 2016-10-27 13:10 UTC (permalink / raw)
  To: Ingo Molnar; +Cc: x86, linux-kernel


Add a sanity check to ensure the stack only grows down, and print a
warning if the check fails.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/unwind_frame.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 5639db6..fe4d387 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
+static 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))
+		return sizeof(*regs) - 2*sizeof(long);
+
+	return sizeof(*regs);
+}
+
 static bool is_last_task_frame(struct unwind_state *state)
 {
 	unsigned long bp = (unsigned long)state->bp;
@@ -79,6 +88,7 @@ bool unwind_next_frame(struct unwind_state *state)
 	struct pt_regs *regs;
 	unsigned long *next_bp, *next_frame;
 	size_t next_len;
+	enum stack_type prev_type = state->stack_info.type;
 
 	if (unwind_done(state))
 		return false;
@@ -142,6 +152,15 @@ bool unwind_next_frame(struct unwind_state *state)
 		goto bad_address;
 	}
 
+	/* Make sure it only unwinds up and doesn't overlap the last frame: */
+	if (state->stack_info.type == prev_type) {
+		if (state->regs && (void *)next_frame < (void *)state->regs + regs_size(state->regs))
+			goto bad_address;
+
+		if (state->bp && (void *)next_frame < (void *)state->bp + FRAME_HEADER_SIZE)
+			goto bad_address;
+	}
+
 	/* move to the next frame */
 	if (regs) {
 		state->regs = regs;
@@ -154,10 +173,17 @@ bool unwind_next_frame(struct unwind_state *state)
 	return true;
 
 bad_address:
-	printk_deferred_once(KERN_WARNING
-		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
-		state->bp, state->task->comm,
-		state->task->pid, next_bp);
+	if (state->regs) {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
+			state->regs, state->task->comm,
+			state->task->pid, next_frame);
+	} else {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
+			state->bp, state->task->comm,
+			state->task->pid, next_frame);
+	}
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [tip:x86/asm] x86/unwind: Ensure stack grows down
  2016-10-27 13:10     ` [PATCH v2] " Josh Poimboeuf
@ 2016-10-28  6:49       ` tip-bot for Josh Poimboeuf
  0 siblings, 0 replies; 11+ messages in thread
From: tip-bot for Josh Poimboeuf @ 2016-10-28  6:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: torvalds, luto, dvlasenk, peterz, bp, linux-kernel, jpoimboe,
	brgerst, mingo, hpa, tglx

Commit-ID:  24d86f59093b0bcb3756cdf47f2db10ff4e90dbb
Gitweb:     http://git.kernel.org/tip/24d86f59093b0bcb3756cdf47f2db10ff4e90dbb
Author:     Josh Poimboeuf <jpoimboe@redhat.com>
AuthorDate: Thu, 27 Oct 2016 08:10:58 -0500
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Fri, 28 Oct 2016 08:16:45 +0200

x86/unwind: Ensure stack grows down

Add a sanity check to ensure the stack only grows down, and print a
warning if the check fails.

Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20161027131058.tpdffwlqipv7pcd6@treble
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/unwind_frame.c | 34 ++++++++++++++++++++++++++++++----
 1 file changed, 30 insertions(+), 4 deletions(-)

diff --git a/arch/x86/kernel/unwind_frame.c b/arch/x86/kernel/unwind_frame.c
index 5639db6..ea7b7f9 100644
--- a/arch/x86/kernel/unwind_frame.c
+++ b/arch/x86/kernel/unwind_frame.c
@@ -32,6 +32,15 @@ unsigned long unwind_get_return_address(struct unwind_state *state)
 }
 EXPORT_SYMBOL_GPL(unwind_get_return_address);
 
+static 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))
+		return sizeof(*regs) - 2*sizeof(long);
+
+	return sizeof(*regs);
+}
+
 static bool is_last_task_frame(struct unwind_state *state)
 {
 	unsigned long bp = (unsigned long)state->bp;
@@ -79,6 +88,7 @@ bool unwind_next_frame(struct unwind_state *state)
 	struct pt_regs *regs;
 	unsigned long *next_bp, *next_frame;
 	size_t next_len;
+	enum stack_type prev_type = state->stack_info.type;
 
 	if (unwind_done(state))
 		return false;
@@ -142,6 +152,15 @@ bool unwind_next_frame(struct unwind_state *state)
 		goto bad_address;
 	}
 
+	/* Make sure it only unwinds up and doesn't overlap the last frame: */
+	if (state->stack_info.type == prev_type) {
+		if (state->regs && (void *)next_frame < (void *)state->regs + regs_size(state->regs))
+			goto bad_address;
+
+		if (state->bp && (void *)next_frame < (void *)state->bp + FRAME_HEADER_SIZE)
+			goto bad_address;
+	}
+
 	/* move to the next frame */
 	if (regs) {
 		state->regs = regs;
@@ -154,10 +173,17 @@ bool unwind_next_frame(struct unwind_state *state)
 	return true;
 
 bad_address:
-	printk_deferred_once(KERN_WARNING
-		"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
-		state->bp, state->task->comm,
-		state->task->pid, next_bp);
+	if (state->regs) {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack regs at %p in %s:%d has bad 'bp' value %p\n",
+			state->regs, state->task->comm,
+			state->task->pid, next_frame);
+	} else {
+		printk_deferred_once(KERN_WARNING
+			"WARNING: kernel stack frame pointer at %p in %s:%d has bad value %p\n",
+			state->bp, state->task->comm,
+			state->task->pid, next_frame);
+	}
 the_end:
 	state->stack_info.type = STACK_TYPE_UNKNOWN;
 	return false;

^ permalink raw reply related	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-10-28  6:50 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-10-26 15:41 [PATCH 0/4] x86/unwind: add some unwinder warnings Josh Poimboeuf
2016-10-26 15:41 ` [PATCH 1/4] x86/unwind: warn on bad frame pointer Josh Poimboeuf
2016-10-27  7:37   ` [tip:x86/asm] x86/unwind: Warn " tip-bot for Josh Poimboeuf
2016-10-26 15:41 ` [PATCH 2/4] x86/unwind: ensure stack grows down Josh Poimboeuf
2016-10-27  6:30   ` Ingo Molnar
2016-10-27 13:10     ` [PATCH v2] " Josh Poimboeuf
2016-10-28  6:49       ` [tip:x86/asm] x86/unwind: Ensure " tip-bot for Josh Poimboeuf
2016-10-26 15:41 ` [PATCH 3/4] x86/dumpstack: warn on stack recursion Josh Poimboeuf
2016-10-27  7:38   ` [tip:x86/asm] x86/dumpstack: Warn " tip-bot for Josh Poimboeuf
2016-10-26 15:41 ` [PATCH 4/4] x86/unwind: detect bad stack return address Josh Poimboeuf
2016-10-27  7:38   ` [tip:x86/asm] x86/unwind: Detect " tip-bot for Josh Poimboeuf

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.