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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.07.18 at 21:15, <andrew.cooper3@xxxxxxxxxx> 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@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).

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