[v2,1/2] x86/traps: guard top-of-stack reads

Commit Message

Jan Beulich June 17, 2019, 8:12 a.m. UTC

Nothing (afaics) guarantees that the original frame's stack pointer
points at readable memory. Avoid a (likely nested) crash by attaching
exception recovery to the read (making it a single read at the same
time). Don't even invoke _show_trace() in case of a non-readable top
slot.

Signed-off-by: Jan Beulich <[e-mail]>
---
v2: Name asm() arguments. Use explicit "fault" variable.

Comments

Andrew Cooper July 2, 2019, 5:47 p.m. UTC | #1

On 17/06/2019 09:12, Jan Beulich wrote:
> Nothing (afaics) guarantees that the original frame's stack pointer

I'd drop the (afaics), because the answer really is nothing.

> points at readable memory. Avoid a (likely nested) crash by attaching
> exception recovery to the read (making it a single read at the same
> time). Don't even invoke _show_trace() in case of a non-readable top
> slot.
>
> Signed-off-by: Jan Beulich <[e-mail]>

Reviewed-by: Andrew Cooper <[e-mail]>

Jan Beulich July 3, 2019, 7:10 a.m. UTC | #2

On 02.07.2019 19:47, Andrew Cooper wrote:
> On 17/06/2019 09:12, Jan Beulich wrote:
>> Nothing (afaics) guarantees that the original frame's stack pointer
> 
> I'd drop the (afaics), because the answer really is nothing.

Well, okay, done.

>> points at readable memory. Avoid a (likely nested) crash by attaching
>> exception recovery to the read (making it a single read at the same
>> time). Don't even invoke _show_trace() in case of a non-readable top
>> slot.
>>
>> Signed-off-by: Jan Beulich <[e-mail]>
> 
> Reviewed-by: Andrew Cooper <[e-mail]>

Thanks. FTR - there's a quirk in here that I've left in place
deliberately (should probably have mentioned it in a post-commit-
message remark) which gets resolved by patch 2, and hence I'm
likely going to wait with committing this such that both can go in
at the same time. The issue is with the if/else-if/else chain here,
which patch 2 makes into a series of plain if()-s. Handling this
correctly right here would imo mean folding together both patches;
anything else would at best result in clumsy intermediate code.
Despite this quirk the change here is an improvement, just not as
much of one as would be desirable.

Jan

Patch

--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -484,17 +484,31 @@  static void _show_trace(unsigned long sp
 
 static void show_trace(const struct cpu_user_regs *regs)
 {
-    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs);
+    unsigned long *sp = ESP_BEFORE_EXCEPTION(regs), tos = 0;
+    bool fault = false;
 
     printk("Xen call trace:\n");
 
+    /* Guarded read of the stack top. */
+    asm ( "1: mov %[data], %[tos]; 2:\n"
+          ".pushsection .fixup,\"ax\"\n"
+          "3: movb $1, %[fault]; jmp 2b\n"
+          ".popsection\n"
+          _ASM_EXTABLE(1b, 3b)
+          : [tos] "+r" (tos), [fault] "+qm" (fault) : [data] "m" (*sp) );
+
     /*
      * If RIP looks sensible, or the top of the stack doesn't, print RIP at
      * the top of the stack trace.
      */
     if ( is_active_kernel_text(regs->rip) ||
-         !is_active_kernel_text(*sp) )
+         !is_active_kernel_text(tos) )
         printk("   [<%p>] %pS\n", _p(regs->rip), _p(regs->rip));
+    else if ( fault )
+    {
+        printk("   [Fault on access]\n");
+        return;
+    }
     /*
      * Else RIP looks bad but the top of the stack looks good.  Perhaps we
      * followed a wild function pointer? Lets assume the top of the stack is a
@@ -503,7 +517,7 @@  static void show_trace(const struct cpu_
      */
     else
     {
-        printk("   [<%p>] %pS\n", _p(*sp), _p(*sp));
+        printk("   [<%p>] %pS\n", _p(tos), _p(tos));
         sp++;
     }