x86: PTI dumpstack fixes
Subsystems (3)
cover x86: PTI dumpstack fixes
A couple of stack dump fixes, which will help debugging PTI issues.
Josh Poimboeuf (2):
x86/dumpstack: Fix partial register dumps x86/dumpstack: Print registers for first stack frame
arch/x86/include/asm/unwind.h | 17 +++++++++++++---- arch/x86/kernel/dumpstack.c | 31 ++++++++++++++++++++++--------- arch/x86/kernel/stacktrace.c | 2 +- 3 files changed, 36 insertions(+), 14 deletions(-)
Signature · 1 line
2.13.6
PATCH 1/2 x86/dumpstack: Fix partial register dumps
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.
--- 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(-)
arch/x86/include/asm/unwind.h +13 -4
@@ -56,18 +56,27 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,#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 ; }
arch/x86/kernel/dumpstack.c +20 -8
@@ -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_struct *task, struct pt_regs *regs,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_struct *task, struct pt_regs *regs,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_struct *task, struct pt_regs *regs,/* * 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 == ®s->ip) goto next;
@@ -199,9 +211,9 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,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)
arch/x86/kernel/stacktrace.c +1 -1
@@ -102,7 +102,7 @@ __save_stack_trace_reliable(struct stack_trace *trace,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
Signature · 1 line
2.13.6
tip-bot for Josh Poimboeuf
merge bot[tip:x86/pti] x86/dumpstack: Fix partial register dumps
x86/dumpstack: Fix partial register dumps
Commit-ID: a9cdbe72c4e8bf3b38781c317a79326e2e1a230d Gitweb: https://git.kernel.org/tip/a9cdbe72c4e8bf3b38781c317a79326e2e1a230d Author: Josh Poimboeuf <[email protected]> AuthorDate: Sun, 31 Dec 2017 10:18:06 -0600 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 3 Jan 2018 16:14:46 +0100
x86/dumpstack: Fix partial register dumps
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.
--- 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(-)
arch/x86/include/asm/unwind.h +13 -4
@@ -56,18 +56,27 @@ void unwind_start(struct unwind_state *state, struct task_struct *task,#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 ; }
arch/x86/kernel/dumpstack.c +20 -8
@@ -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_struct *task, struct pt_regs *regs,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_struct *task, struct pt_regs *regs,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_struct *task, struct pt_regs *regs,/* * 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 == ®s->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)
arch/x86/kernel/stacktrace.c +1 -1
@@ -98,7 +98,7 @@ static int __save_stack_trace_reliable(struct stack_trace *trace,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
PATCH 2/2 x86/dumpstack: Print registers for first stack frame
In reply to Josh PoimboeufIn the stack dump code, if the frame after the starting pt_regs is also a regs frame, the registers don't get printed. Fix that.
--- arch/x86/kernel/dumpstack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/dumpstack.c +2 -1
@@ -115,6 +115,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,unwind_start (&state, task, regs, stack); stack = stack ? :get_stack_pointer (task, regs); + regs =unwind_get_entry_regs (&state, &partial);/* * Iterate through the stacks, starting with the current stack pointer.
@@ -132,7 +133,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,* - hardirq stack * - entry stack */ -for (regs =NULL ; stack; stack =PTR_ALIGN (stack_info.next_sp,sizeof (long ))) { +for ( ; stack; stack =PTR_ALIGN (stack_info.next_sp,sizeof (long ))) {const char *stack_name;if (get_stack_info (stack, task, &stack_info, &visit_mask)) {
Signature · 1 line
2.13.6
tip-bot for Josh Poimboeuf
merge bot[tip:x86/pti] x86/dumpstack: Print registers for first stack frame
x86/dumpstack: Print registers for first stack frame
Commit-ID: 3ffdeb1a02be3086f1411a15c5b9c481fa28e21f Gitweb: https://git.kernel.org/tip/3ffdeb1a02be3086f1411a15c5b9c481fa28e21f Author: Josh Poimboeuf <[email protected]> AuthorDate: Sun, 31 Dec 2017 10:18:07 -0600 Committer: Ingo Molnar <[email protected]> CommitDate: Wed, 3 Jan 2018 16:14:46 +0100
x86/dumpstack: Print registers for first stack frame
In the stack dump code, if the frame after the starting pt_regs is also a regs frame, the registers don't get printed. Fix that.
--- arch/x86/kernel/dumpstack.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/x86/kernel/dumpstack.c +2 -1
@@ -115,6 +115,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,unwind_start (&state, task, regs, stack); stack = stack ? :get_stack_pointer (task, regs); + regs =unwind_get_entry_regs (&state, &partial);/* * Iterate through the stacks, starting with the current stack pointer.
@@ -132,7 +133,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs,* - hardirq stack * - entry stack */ -for (regs =NULL ; stack; stack =PTR_ALIGN (stack_info.next_sp,sizeof (long ))) { +for ( ; stack; stack =PTR_ALIGN (stack_info.next_sp,sizeof (long ))) {const char *stack_name;if (get_stack_info (stack, task, &stack_info, &visit_mask)) {