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

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

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH] x86/traps: Fix cascade crash in show_registers()

[Jan Beulich]

>>> On 25.07.18 at 21:15, <andrew.cooper3@citrix.com> wrote:
> 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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> 
> 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).

I'm having difficulty connecting stack alignment and the presence or
absence of these fields (it's four 64-bit slots you'd get rid of, which has no
effect at all on overall alignment afaics), or use of SIMD insns. Could you
enlighten me?

> --- 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));

I don't really like the open-coded reference to es - could I talk you into
splitting out a macro, the definition of which would be put next to
get_stack_bottom()'s? The more that there's already a similar open-
coded use in load_system_tables(). Other than that I'm fine with this,
no matter whether as a temporary or a permanent measure.

Jan



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