Re: [PATCH 10/16] x86/cpu: Adjust reset_stack_and_jump() to be shadow stack compatible

[Jan Beulich <jbeulich@suse.com>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> We need to unwind up to the supervisor token.  See the comment for details.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/include/asm-x86/current.h | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h
> index 99b66a0087..2a7b728b1e 100644
> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -124,13 +124,49 @@ unsigned long get_stack_dump_bottom (unsigned long sp);
>  # define CHECK_FOR_LIVEPATCH_WORK ""
>  #endif
>  
> +#ifdef CONFIG_XEN_SHSTK
> +/*
> + * We need to unwind the primary shadow stack to its supervisor token, located
> + * at 0x5ff8 from the base of the stack blocks.
> + *
> + * Read the shadow stack pointer, subtract it from 0x5ff8, divide by 8 to get
> + * the number of slots needing popping.
> + *
> + * INCSSPQ can't pop more than 255 entries.  We shouldn't ever need to pop
> + * that many entries, and getting this wrong will cause us to #DF later.
> + */
> +# define SHADOW_STACK_WORK                      \
> +    "mov $1, %[ssp];"                           \
> +    "rdsspd %[ssp];"                            \
> +    "cmp $1, %[ssp];"                           \
> +    "je 1f;" /* CET not active?  Skip. */       \
> +    "mov $"STR(0x5ff8)", %[val];"               \

As per comments on earlier patches, I think it would be nice if
this wasn't a literal number here, but tied to actual stack
layout via some suitable expression. An option might be to use
0xff8 (or the constant to be introduced for it in the earlier
patch) here and ...

> +    "and $"STR(STACK_SIZE - 1)", %[ssp];"       \

... PAGE_SIZE here.

> +    "sub %[ssp], %[val];"                       \
> +    "shr $3, %[val];"                           \
> +    "cmp $255, %[val];"                         \
> +    "jle 2f;"                                   \

Perhaps better "jbe", treating the unsigned values as such?

> +    "ud2a;"                                     \
> +    "2: incsspq %q[val];"                       \
> +    "1:"
> +#else
> +# define SHADOW_STACK_WORK ""
> +#endif
> +
>  #define switch_stack_and_jump(fn, instr)                                \
>      ({                                                                  \
> +        unsigned int tmp;                                               \
>          __asm__ __volatile__ (                                          \
> -            "mov %0,%%"__OP"sp;"                                        \
> +            "cmc;"                                                      \
> +            SHADOW_STACK_WORK                                           \
> +            "mov %[stk], %%rsp;"                                        \
>              instr                                                       \
> -             "jmp %c1"                                                  \
> -            : : "r" (guest_cpu_user_regs()), "i" (fn) : "memory" );     \
> +            "jmp %c[fun];"                                              \
> +            : [val] "=&r" (tmp),                                        \
> +              [ssp] "=&r" (tmp)                                         \

See my concern on the earlier similar construct.

Jan