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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> When executing an IRET-to-self, the shadow stack must agree with the regular
> stack.  We can't manipulate SSP directly, so have to fake a shadow IRET frame
> by executing 3 CALLs, then editing the result to look correct.
> 
> This is not a fastpath, is called on the BSP long before CET can be set up,
> and may be called on the crash path after CET is disabled.  Use the fact that
> INCSSP is allocated from the hint nop space to construct a test for CET being
> active which is safe on all processors.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
albeit with a question which may make a further change necessary:

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -544,17 +544,40 @@ static inline void enable_nmis(void)
>  {
>      unsigned long tmp;
>  
> -    asm volatile ( "mov %%rsp, %[tmp]     \n\t"
> -                   "push %[ss]            \n\t"
> -                   "push %[tmp]           \n\t"
> -                   "pushf                 \n\t"
> -                   "push %[cs]            \n\t"
> -                   "lea 1f(%%rip), %[tmp] \n\t"
> -                   "push %[tmp]           \n\t"
> -                   "iretq; 1:             \n\t"
> -                   : [tmp] "=&r" (tmp)
> +    asm volatile ( "mov     %%rsp, %[rsp]        \n\t"
> +                   "lea    .Ldone(%%rip), %[rip] \n\t"
> +#ifdef CONFIG_XEN_SHSTK
> +                   /* Check for CET-SS being active. */
> +                   "mov    $1, %k[ssp]           \n\t"
> +                   "rdsspq %[ssp]                \n\t"
> +                   "cmp    $1, %k[ssp]           \n\t"
> +                   "je     .Lshstk_done          \n\t"
> +
> +                   /* Push 3 words on the shadow stack */
> +                   ".rept 3                      \n\t"
> +                   "call 1f; nop; 1:             \n\t"
> +                   ".endr                        \n\t"
> +
> +                   /* Fixup to be an IRET shadow stack frame */
> +                   "wrssq  %q[cs], -1*8(%[ssp])  \n\t"
> +                   "wrssq  %[rip], -2*8(%[ssp])  \n\t"
> +                   "wrssq  %[ssp], -3*8(%[ssp])  \n\t"
> +
> +                   ".Lshstk_done:"
> +#endif
> +                   /* Write an IRET regular frame */
> +                   "push   %[ss]                 \n\t"
> +                   "push   %[rsp]                \n\t"
> +                   "pushf                        \n\t"
> +                   "push   %q[cs]                \n\t"
> +                   "push   %[rip]                \n\t"
> +                   "iretq                        \n\t"
> +                   ".Ldone:                      \n\t"
> +                   : [rip] "=&r" (tmp),
> +                     [rsp] "=&r" (tmp),
> +                     [ssp] "=&r" (tmp)

Even after an hour of reading and searching through the gcc docs
I can't convince myself that this utilizes defined behavior. We
do tie multiple outputs to the same C variable elsewhere, yes,
but only in cases where we don't really care about the register,
or where the register is a fixed one anyway. What I can't find
is a clear statement that gcc wouldn't ever, now or in the
future, use the same register for all three outputs. I think one
can deduce this in certain ways, and experimentation also seems
to confirm it, but still.

Jan