]>
Commit | Line | Data |
---|---|---|
035dbe67 FG |
1 | From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 |
2 | From: Josh Poimboeuf <jpoimboe@redhat.com> | |
3 | Date: Sun, 31 Dec 2017 10:18:06 -0600 | |
4 | Subject: [PATCH] x86/dumpstack: Fix partial register dumps | |
5 | MIME-Version: 1.0 | |
6 | Content-Type: text/plain; charset=UTF-8 | |
7 | Content-Transfer-Encoding: 8bit | |
8 | ||
9 | CVE-2017-5754 | |
10 | ||
11 | The show_regs_safe() logic is wrong. When there's an iret stack frame, | |
12 | it prints the entire pt_regs -- most of which is random stack data -- | |
13 | instead of just the five registers at the end. | |
14 | ||
15 | show_regs_safe() is also poorly named: the on_stack() checks aren't for | |
16 | safety. Rename the function to show_regs_if_on_stack() and add a | |
17 | comment to explain why the checks are needed. | |
18 | ||
19 | These issues were introduced with the "partial register dump" feature of | |
20 | the following commit: | |
21 | ||
22 | b02fcf9ba121 ("x86/unwinder: Handle stack overflows more gracefully") | |
23 | ||
24 | That patch had gone through a few iterations of development, and the | |
25 | above issues were artifacts from a previous iteration of the patch where | |
26 | 'regs' pointed directly to the iret frame rather than to the (partially | |
27 | empty) pt_regs. | |
28 | ||
29 | Tested-by: Alexander Tsoy <alexander@tsoy.me> | |
30 | Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com> | |
31 | Cc: Andy Lutomirski <luto@kernel.org> | |
32 | Cc: Linus Torvalds <torvalds@linux-foundation.org> | |
33 | Cc: Peter Zijlstra <peterz@infradead.org> | |
34 | Cc: Thomas Gleixner <tglx@linutronix.de> | |
35 | Cc: Toralf Förster <toralf.foerster@gmx.de> | |
36 | Cc: stable@vger.kernel.org | |
37 | Fixes: b02fcf9ba121 ("x86/unwinder: Handle stack overflows more gracefully") | |
38 | Link: http://lkml.kernel.org/r/5b05b8b344f59db2d3d50dbdeba92d60f2304c54.1514736742.git.jpoimboe@redhat.com | |
39 | Signed-off-by: Ingo Molnar <mingo@kernel.org> | |
40 | (cherry picked from commit a9cdbe72c4e8bf3b38781c317a79326e2e1a230d) | |
41 | Signed-off-by: Andy Whitcroft <apw@canonical.com> | |
42 | Signed-off-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com> | |
43 | (cherry picked from commit 3f159d02ecca1ffe81dc467767833dd6d0345147) | |
44 | Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> | |
45 | --- | |
46 | arch/x86/include/asm/unwind.h | 17 +++++++++++++---- | |
47 | arch/x86/kernel/dumpstack.c | 28 ++++++++++++++++++++-------- | |
48 | arch/x86/kernel/stacktrace.c | 2 +- | |
49 | 3 files changed, 34 insertions(+), 13 deletions(-) | |
50 | ||
51 | diff --git a/arch/x86/include/asm/unwind.h b/arch/x86/include/asm/unwind.h | |
52 | index 38fa6154e382..e1c1cb5019bc 100644 | |
53 | --- a/arch/x86/include/asm/unwind.h | |
54 | +++ b/arch/x86/include/asm/unwind.h | |
55 | @@ -55,18 +55,27 @@ void unwind_start(struct unwind_state *state, struct task_struct *task, | |
56 | ||
57 | #if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER) | |
58 | /* | |
59 | - * WARNING: The entire pt_regs may not be safe to dereference. In some cases, | |
60 | - * only the iret frame registers are accessible. Use with caution! | |
61 | + * If 'partial' returns true, only the iret frame registers are valid. | |
62 | */ | |
63 | -static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state) | |
64 | +static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state, | |
65 | + bool *partial) | |
66 | { | |
67 | if (unwind_done(state)) | |
68 | return NULL; | |
69 | ||
70 | + if (partial) { | |
71 | +#ifdef CONFIG_UNWINDER_ORC | |
72 | + *partial = !state->full_regs; | |
73 | +#else | |
74 | + *partial = false; | |
75 | +#endif | |
76 | + } | |
77 | + | |
78 | return state->regs; | |
79 | } | |
80 | #else | |
81 | -static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state) | |
82 | +static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state, | |
83 | + bool *partial) | |
84 | { | |
85 | return NULL; | |
86 | } | |
87 | diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c | |
88 | index 19a936e9b259..8da5b487919f 100644 | |
89 | --- a/arch/x86/kernel/dumpstack.c | |
90 | +++ b/arch/x86/kernel/dumpstack.c | |
91 | @@ -76,12 +76,23 @@ void show_iret_regs(struct pt_regs *regs) | |
92 | regs->sp, regs->flags); | |
93 | } | |
94 | ||
95 | -static void show_regs_safe(struct stack_info *info, struct pt_regs *regs) | |
96 | +static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs, | |
97 | + bool partial) | |
98 | { | |
99 | - if (on_stack(info, regs, sizeof(*regs))) | |
100 | + /* | |
101 | + * These on_stack() checks aren't strictly necessary: the unwind code | |
102 | + * has already validated the 'regs' pointer. The checks are done for | |
103 | + * ordering reasons: if the registers are on the next stack, we don't | |
104 | + * want to print them out yet. Otherwise they'll be shown as part of | |
105 | + * the wrong stack. Later, when show_trace_log_lvl() switches to the | |
106 | + * next stack, this function will be called again with the same regs so | |
107 | + * they can be printed in the right context. | |
108 | + */ | |
109 | + if (!partial && on_stack(info, regs, sizeof(*regs))) { | |
110 | __show_regs(regs, 0); | |
111 | - else if (on_stack(info, (void *)regs + IRET_FRAME_OFFSET, | |
112 | - IRET_FRAME_SIZE)) { | |
113 | + | |
114 | + } else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET, | |
115 | + IRET_FRAME_SIZE)) { | |
116 | /* | |
117 | * When an interrupt or exception occurs in entry code, the | |
118 | * full pt_regs might not have been saved yet. In that case | |
119 | @@ -98,6 +109,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, | |
120 | struct stack_info stack_info = {0}; | |
121 | unsigned long visit_mask = 0; | |
122 | int graph_idx = 0; | |
123 | + bool partial; | |
124 | ||
125 | printk("%sCall Trace:\n", log_lvl); | |
126 | ||
127 | @@ -140,7 +152,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, | |
128 | printk("%s <%s>\n", log_lvl, stack_name); | |
129 | ||
130 | if (regs) | |
131 | - show_regs_safe(&stack_info, regs); | |
132 | + show_regs_if_on_stack(&stack_info, regs, partial); | |
133 | ||
134 | /* | |
135 | * Scan the stack, printing any text addresses we find. At the | |
136 | @@ -164,7 +176,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, | |
137 | ||
138 | /* | |
139 | * Don't print regs->ip again if it was already printed | |
140 | - * by show_regs_safe() below. | |
141 | + * by show_regs_if_on_stack(). | |
142 | */ | |
143 | if (regs && stack == ®s->ip) { | |
144 | unwind_next_frame(&state); | |
145 | @@ -200,9 +212,9 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, | |
146 | unwind_next_frame(&state); | |
147 | ||
148 | /* if the frame has entry regs, print them */ | |
149 | - regs = unwind_get_entry_regs(&state); | |
150 | + regs = unwind_get_entry_regs(&state, &partial); | |
151 | if (regs) | |
152 | - show_regs_safe(&stack_info, regs); | |
153 | + show_regs_if_on_stack(&stack_info, regs, partial); | |
154 | } | |
155 | ||
156 | if (stack_name) | |
157 | diff --git a/arch/x86/kernel/stacktrace.c b/arch/x86/kernel/stacktrace.c | |
158 | index 8dabd7bf1673..60244bfaf88f 100644 | |
159 | --- a/arch/x86/kernel/stacktrace.c | |
160 | +++ b/arch/x86/kernel/stacktrace.c | |
161 | @@ -98,7 +98,7 @@ static int __save_stack_trace_reliable(struct stack_trace *trace, | |
162 | for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state); | |
163 | unwind_next_frame(&state)) { | |
164 | ||
165 | - regs = unwind_get_entry_regs(&state); | |
166 | + regs = unwind_get_entry_regs(&state, NULL); | |
167 | if (regs) { | |
168 | /* | |
169 | * Kernel mode registers on the stack indicate an | |
170 | -- | |
171 | 2.14.2 | |
172 |