[Xen-devel] [PATCH] x86/traps: Fix cascade crash in show_registers()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

Copying all of struct cpu_user_regs is generally unsafe, as the structure
extends beyond the hardware exception frame.

This generally copies 32 bytes beyond the top of the valid stack frame, and in
the case of the top IST stack, hits the unmapped guard page.

Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
---
CC: Jan Beulich <JBeulich@xxxxxxxx>

RFC, because I don't necesserily like this fix.  It is disappointing that
cpu_user_regs is in the public API because it has no real buisness living
there.

Now that we are 64bit only, the vm86 fields are never used in stack frames,
and only really used in struct vcpu when out of current context.  A different
fix would be to respecify a new structure which is internal to Xen and stops
at the hardware frame, and add a tiny bit of compat glue for the hypercalls
which use struct cpu_user_regs.

The end result of this would be that regs-> doesn't have any fields which can
point off the valid stack, and we can actually fix the stack alignment
requirements for EFI and avoid the dubious bodge in c/s f6b7fedc8.  It will
also allow us to more sensibly use AVX/AVX512 in Xen (think optimised
idle-loop scrubbing).

Thoughts?
---
 xen/arch/x86/x86_64/traps.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/x86_64/traps.c b/xen/arch/x86/x86_64/traps.c
index ed02b78..70b889a 100644
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -95,11 +95,18 @@ static void _show_registers(
 
 void show_registers(const struct cpu_user_regs *regs)
 {
-    struct cpu_user_regs fault_regs = *regs;
+    struct cpu_user_regs fault_regs;
     unsigned long fault_crs[8];
     enum context context;
     struct vcpu *v = system_state >= SYS_STATE_smp_boot ? current : NULL;
 
+    /*
+     * Copy up to the top of the hardware exception frame.  When in IST
+     * context, the vm86 fields aren't valid and point into the adjacent
+     * stack, which in the case of the highest IST is the unmapped guard page.
+     */
+    memcpy(&fault_regs, regs, offsetof(struct cpu_user_regs, es));
+
     if ( guest_mode(regs) && is_hvm_vcpu(v) )
     {
         struct segment_register sreg;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel