Re: [PATCH 16/16] x86/shstk: Activate Supervisor Shadow Stacks

[Jan Beulich <jbeulich@xxxxxxxx>]

On 02.05.2020 00:58, Andrew Cooper wrote:
> --- a/xen/arch/x86/acpi/wakeup_prot.S
> +++ b/xen/arch/x86/acpi/wakeup_prot.S
> @@ -1,3 +1,8 @@
> +#include <asm/config.h>

Why is this needed? Afaics assembly files, just like C ones, get
xen/config.h included from the compiler command line.

> @@ -48,6 +59,48 @@ ENTRY(s3_resume)
>          pushq   %rax
>          lretq
>  1:
> +#ifdef CONFIG_XEN_SHSTK
> +     /*
> +         * Restoring SSP is a little convoluted, because we are intercepting
> +         * the middle of an in-use shadow stack.  Write a temporary 
> supervisor
> +         * token under the stack, so SETSSBSY takes us where we want, then
> +         * reset MSR_PL0_SSP to its usual value and pop the temporary token.

What do you mean by "takes us where we want"? I take it "us" is really
SSP here?

> +         */
> +        mov     saved_rsp(%rip), %rdi
> +        cmpq    $1, %rdi
> +        je      .L_shstk_done
> +
> +        /* Write a supervisor token under SSP. */
> +        sub     $8, %rdi
> +        mov     %rdi, (%rdi)
> +
> +        /* Load it into MSR_PL0_SSP. */
> +        mov     $MSR_PL0_SSP, %ecx
> +        mov     %rdi, %rdx
> +        shr     $32, %rdx
> +        mov     %edi, %eax
> +
> +        /* Enable CET. */
> +        mov     $MSR_S_CET, %ecx
> +        xor     %edx, %edx
> +        mov     $CET_SHSTK_EN | CET_WRSS_EN, %eax
> +        wrmsr
> +
> +        /* Activate our temporary token. */
> +        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx
> +        mov     %rbx, %cr4
> +        setssbsy
> +
> +        /* Reset MSR_PL0_SSP back to its expected value. */
> +        and     $~(STACK_SIZE - 1), %eax
> +        or      $0x5ff8, %eax
> +        wrmsr

Ahead of this WRMSR neither %ecx nor %edx look to have their intended
values anymore. Also there is a again a magic 0x5ff8 here (and at
least one more further down).

> --- a/xen/arch/x86/boot/x86_64.S
> +++ b/xen/arch/x86/boot/x86_64.S
> @@ -28,8 +28,36 @@ ENTRY(__high_start)
>          lretq
>  1:
>          test    %ebx,%ebx
> -        jnz     start_secondary
> +        jz      .L_bsp
>  
> +        /* APs.  Set up shadow stacks before entering C. */
> +
> +        testl   $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \
> +                CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + 
> boot_cpu_data(%rip)
> +        je      .L_ap_shstk_done
> +
> +        mov     $MSR_S_CET, %ecx
> +        xor     %edx, %edx
> +        mov     $CET_SHSTK_EN | CET_WRSS_EN, %eax
> +        wrmsr
> +
> +        mov     $MSR_PL0_SSP, %ecx
> +        mov     %rsp, %rdx
> +        shr     $32, %rdx
> +        mov     %esp, %eax
> +        and     $~(STACK_SIZE - 1), %eax
> +        or      $0x5ff8, %eax
> +        wrmsr
> +
> +        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
> +        mov     %rcx, %cr4
> +        setssbsy

Since the token doesn't get written here, could you make the comment
say where this happens? I have to admit that I had to go through
earlier patches to find it again.

> +.L_ap_shstk_done:
> +        call    start_secondary
> +        BUG     /* start_secondary() shouldn't return. */

This conversion from a jump to CALL is unrelated and hence would
better be mentioned in the description imo.

> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -323,6 +323,11 @@ void __init early_cpu_init(void)
>              x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86,
>              c->x86_model, c->x86_model, c->x86_mask, eax);
>  
> +     if (c->cpuid_level >= 7) {
> +             cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
> +             c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx;
> +     }

How about moving the leaf 7 code from generic_identify() here as
a whole?

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -664,6 +664,13 @@ static void __init noreturn reinit_bsp_stack(void)
>      stack_base[0] = stack;
>      memguard_guard_stack(stack);
>  
> +    if ( cpu_has_xen_shstk )
> +    {
> +        wrmsrl(MSR_PL0_SSP, (unsigned long)stack + 0x5ff8);
> +        wrmsrl(MSR_S_CET, CET_SHSTK_EN | CET_WRSS_EN);
> +        asm volatile ("setssbsy" ::: "memory");
> +    }

Same as for APs - a brief comment pointing at where the token was
written would seem helpful.

Could you also have the patch description say a word on the choice
of enabling CET_WRSS_EN uniformly and globally?

> @@ -985,6 +992,21 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>      /* This must come before e820 code because it sets paddr_bits. */
>      early_cpu_init();
>  
> +    /* Choose shadow stack early, to set infrastructure up appropriately. */
> +    if ( opt_xen_shstk && boot_cpu_has(X86_FEATURE_CET_SS) )
> +    {
> +        printk("Enabling Supervisor Shadow Stacks\n");
> +
> +        setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK);
> +#ifdef CONFIG_PV32
> +        if ( opt_pv32 )
> +        {
> +            opt_pv32 = 0;
> +            printk("  - Disabling PV32 due to Shadow Stacks\n");
> +        }
> +#endif

I think this deserves an explanation, either in a comment or in
the patch description.

> @@ -1721,6 +1743,10 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      alternative_branches();
>  
> +    /* Defer CR4.CET until alternatives have finished playing with CR4.WP */
> +    if ( cpu_has_xen_shstk )
> +        set_in_cr4(X86_CR4_CET);

Nit: CR0.WP (in the comment)

Jan