linux-kernel.vger.kernel.org archive mirror
 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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).