public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: linux-kernel@vger.kernel.org
Cc: "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	stable@vger.kernel.org, "Alexander Tsoy" <alexander@tsoy.me>,
	"Josh Poimboeuf" <jpoimboe@redhat.com>,
	"Andy Lutomirski" <luto@kernel.org>,
	"Linus Torvalds" <torvalds@linux-foundation.org>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Thomas Gleixner" <tglx@linutronix.de>,
	"Toralf Förster" <toralf.foerster@gmx.de>,
	"Ingo Molnar" <mingo@kernel.org>
Subject: [PATCH 4.14 05/14] x86/dumpstack: Fix partial register dumps
Date: Thu,  4 Jan 2018 13:09:22 +0100	[thread overview]
Message-ID: <20180104120917.781072895@linuxfoundation.org> (raw)
In-Reply-To: <20180104120917.043667757@linuxfoundation.org>

4.14-stable review patch.  If anyone has any objections, please let me know.

------------------

From: Josh Poimboeuf <jpoimboe@redhat.com>

commit a9cdbe72c4e8bf3b38781c317a79326e2e1a230d upstream.

The show_regs_safe() logic is wrong.  When there's an iret stack frame,
it prints the entire pt_regs -- most of which is random stack data --
instead of just the five registers at the end.

show_regs_safe() is also poorly named: the on_stack() checks aren't for
safety.  Rename the function to show_regs_if_on_stack() and add a
comment to explain why the checks are needed.

These issues were introduced with the "partial register dump" feature of
the following commit:

  b02fcf9ba121 ("x86/unwinder: Handle stack overflows more gracefully")

That patch had gone through a few iterations of development, and the
above issues were artifacts from a previous iteration of the patch where
'regs' pointed directly to the iret frame rather than to the (partially
empty) pt_regs.

Tested-by: Alexander Tsoy <alexander@tsoy.me>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Toralf Förster <toralf.foerster@gmx.de>
Fixes: b02fcf9ba121 ("x86/unwinder: Handle stack overflows more gracefully")
Link: http://lkml.kernel.org/r/5b05b8b344f59db2d3d50dbdeba92d60f2304c54.1514736742.git.jpoimboe@redhat.com
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

---
 arch/x86/include/asm/unwind.h |   17 +++++++++++++----
 arch/x86/kernel/dumpstack.c   |   28 ++++++++++++++++++++--------
 arch/x86/kernel/stacktrace.c  |    2 +-
 3 files changed, 34 insertions(+), 13 deletions(-)

--- a/arch/x86/include/asm/unwind.h
+++ b/arch/x86/include/asm/unwind.h
@@ -56,18 +56,27 @@ void unwind_start(struct unwind_state *s
 
 #if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER)
 /*
- * WARNING: The entire pt_regs may not be safe to dereference.  In some cases,
- * only the iret frame registers are accessible.  Use with caution!
+ * If 'partial' returns true, only the iret frame registers are valid.
  */
-static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
+static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state,
+						    bool *partial)
 {
 	if (unwind_done(state))
 		return NULL;
 
+	if (partial) {
+#ifdef CONFIG_UNWINDER_ORC
+		*partial = !state->full_regs;
+#else
+		*partial = false;
+#endif
+	}
+
 	return state->regs;
 }
 #else
-static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)
+static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state,
+						    bool *partial)
 {
 	return NULL;
 }
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -76,12 +76,23 @@ void show_iret_regs(struct pt_regs *regs
 		regs->sp, regs->flags);
 }
 
-static void show_regs_safe(struct stack_info *info, struct pt_regs *regs)
+static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,
+				  bool partial)
 {
-	if (on_stack(info, regs, sizeof(*regs)))
+	/*
+	 * These on_stack() checks aren't strictly necessary: the unwind code
+	 * has already validated the 'regs' pointer.  The checks are done for
+	 * ordering reasons: if the registers are on the next stack, we don't
+	 * want to print them out yet.  Otherwise they'll be shown as part of
+	 * the wrong stack.  Later, when show_trace_log_lvl() switches to the
+	 * next stack, this function will be called again with the same regs so
+	 * they can be printed in the right context.
+	 */
+	if (!partial && on_stack(info, regs, sizeof(*regs))) {
 		__show_regs(regs, 0);
-	else if (on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
-			  IRET_FRAME_SIZE)) {
+
+	} else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,
+				       IRET_FRAME_SIZE)) {
 		/*
 		 * When an interrupt or exception occurs in entry code, the
 		 * full pt_regs might not have been saved yet.  In that case
@@ -98,6 +109,7 @@ void show_trace_log_lvl(struct task_stru
 	struct stack_info stack_info = {0};
 	unsigned long visit_mask = 0;
 	int graph_idx = 0;
+	bool partial;
 
 	printk("%sCall Trace:\n", log_lvl);
 
@@ -140,7 +152,7 @@ void show_trace_log_lvl(struct task_stru
 			printk("%s <%s>\n", log_lvl, stack_name);
 
 		if (regs)
-			show_regs_safe(&stack_info, regs);
+			show_regs_if_on_stack(&stack_info, regs, partial);
 
 		/*
 		 * Scan the stack, printing any text addresses we find.  At the
@@ -164,7 +176,7 @@ void show_trace_log_lvl(struct task_stru
 
 			/*
 			 * Don't print regs->ip again if it was already printed
-			 * by show_regs_safe() below.
+			 * by show_regs_if_on_stack().
 			 */
 			if (regs && stack == &regs->ip)
 				goto next;
@@ -199,9 +211,9 @@ next:
 			unwind_next_frame(&state);
 
 			/* if the frame has entry regs, print them */
-			regs = unwind_get_entry_regs(&state);
+			regs = unwind_get_entry_regs(&state, &partial);
 			if (regs)
-				show_regs_safe(&stack_info, regs);
+				show_regs_if_on_stack(&stack_info, regs, partial);
 		}
 
 		if (stack_name)
--- a/arch/x86/kernel/stacktrace.c
+++ b/arch/x86/kernel/stacktrace.c
@@ -98,7 +98,7 @@ static int __save_stack_trace_reliable(s
 	for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);
 	     unwind_next_frame(&state)) {
 
-		regs = unwind_get_entry_regs(&state);
+		regs = unwind_get_entry_regs(&state, NULL);
 		if (regs) {
 			/*
 			 * Kernel mode registers on the stack indicate an

  parent reply	other threads:[~2018-01-04 12:09 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-04 12:09 [PATCH 4.14 00/14] 4.14.12-stable review Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 01/14] exec: Weaken dumpability for secureexec Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 02/14] capabilities: fix buffer overread on very short xattr Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 03/14] x86/cpu, x86/pti: Do not enable PTI on AMD processors Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 04/14] x86/pti: Make sure the user/kernel PTEs match Greg Kroah-Hartman
2018-01-04 12:09 ` Greg Kroah-Hartman [this message]
2018-01-04 12:09 ` [PATCH 4.14 06/14] x86/dumpstack: Print registers for first stack frame Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 07/14] x86/pti: Switch to kernel CR3 at early in entry_SYSCALL_compat() Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 08/14] x86/process: Define cpu_tss_rw in same section as declaration Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 09/14] Revert "xfrm: Fix stack-out-of-bounds read in xfrm_state_find." Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 10/14] rtc: m41t80: m41t80_sqw_set_rate should return 0 on success Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 11/14] rtc: m41t80: fix m41t80_sqw_round_rate return value Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 12/14] rtc: m41t80: avoid i2c read in m41t80_sqw_recalc_rate Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 13/14] rtc: m41t80: avoid i2c read in m41t80_sqw_is_prepared Greg Kroah-Hartman
2018-01-04 12:09 ` [PATCH 4.14 14/14] rtc: m41t80: remove unneeded checks from m41t80_sqw_set_rate Greg Kroah-Hartman
2018-01-04 17:47 ` [PATCH 4.14 00/14] 4.14.12-stable review kernelci.org bot
2018-01-05  0:12   ` Kevin Hilman
2018-01-05  7:55     ` Greg Kroah-Hartman
2018-01-08 14:58     ` Guillaume Tucker
2018-01-04 18:52 ` Guenter Roeck
2018-01-05 12:13   ` Greg Kroah-Hartman
2018-01-04 19:46 ` Dan Rue
2018-01-05  8:04   ` Greg Kroah-Hartman
2018-01-04 22:03 ` Shuah Khan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180104120917.781072895@linuxfoundation.org \
    --to=gregkh@linuxfoundation.org \
    --cc=alexander@tsoy.me \
    --cc=jpoimboe@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luto@kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=stable@vger.kernel.org \
    --cc=tglx@linutronix.de \
    --cc=toralf.foerster@gmx.de \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox