Re: [PATCH 63/65] x86/setup: Rework MSR_S_CET handling for CET-IBT

[Andrew Cooper <amc96@xxxxxxxx>]

On 06/12/2021 10:49, Jan Beulich wrote:
> On 26.11.2021 13:34, Andrew Cooper wrote:
>> --- a/xen/arch/x86/acpi/wakeup_prot.S
>> +++ b/xen/arch/x86/acpi/wakeup_prot.S
>> @@ -63,7 +63,24 @@ ENTRY(s3_resume)
>>          pushq   %rax
>>          lretq
>>  1:
>> -#ifdef CONFIG_XEN_SHSTK
>> +#if defined(CONFIG_XEN_SHSTK) || defined(CONFIG_XEN_IBT)
>> +        call    xen_msr_s_cet_value
>> +        test    %eax, %eax
>> +        je      .L_cet_done
> Nit: I consider it generally misleading to use JE / JNE (and a few
> other Jcc) with other than CMP-like insns. Only those handle actual
> "relations", whereas e.g. TEST only produces particular flag states,
> so would more consistently be followed by JZ / JNZ in cases like
> this one. But since this is very much a matter of taste, I'm not
> going to insist on a change here.

Fixed.

>
>> +        /* Set up MSR_S_CET. */
>> +        mov     $MSR_S_CET, %ecx
>> +        xor     %edx, %edx
>> +        wrmsr
>> +
>> +        /* Enable CR4.CET. */
>> +        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
>> +        mov     %rcx, %cr4
> Is it valid / safe to enable CR4.CET (with CET_SHSTK_EN already
> active) before ...
>
>> +#if defined(CONFIG_XEN_SHSTK)
>> +        test    $CET_SHSTK_EN, %eax
> (Intermediate remark: Using %al would seem to suffice and be a
> shorter encoding.)

Fixed.

>
>> +        je      .L_cet_done
>> +
>>          /*
>>           * Restoring SSP is a little complicated, because we are 
>> intercepting
>>           * an in-use shadow stack.  Write a temporary token under the stack,
>> @@ -71,14 +88,6 @@ ENTRY(s3_resume)
>>           * reset MSR_PL0_SSP to its usual value and pop the temporary token.
>>           */
>>          mov     saved_ssp(%rip), %rdi
>> -        cmpq    $1, %rdi
>> -        je      .L_shstk_done
>> -
>> -        /* Set up MSR_S_CET. */
>> -        mov     $MSR_S_CET, %ecx
>> -        xor     %edx, %edx
>> -        mov     $CET_SHSTK_EN | CET_WRSS_EN, %eax
>> -        wrmsr
>>  
>>          /* Construct the temporary supervisor token under SSP. */
>>          sub     $8, %rdi
>> @@ -90,12 +99,9 @@ ENTRY(s3_resume)
>>          mov     %edi, %eax
>>          wrmsr
>>  
>> -        /* Enable CET.  MSR_INTERRUPT_SSP_TABLE is set up later in 
>> load_system_tables(). */
>> -        mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx
>> -        mov     %rbx, %cr4
> ... the writing of MSR_PL0_SSP in context here? ISTR some ordering
> issues back at the time when you introduced CET-SS, so I thought I'd
> better ask to be sure.

Yes, it is safe, but the reasons why aren't entirely trivial.

To set up CET-SS, we need to do the following things:

1) CR4.CET=1
2) Configure MSR_S_CET.SHSTK_EN
3) Configure MSR_PL0_SSP pointing at a non-busy supervisor token
4) Configure MSR_ISST_SSP to point at the IST shadow stacks, again with
non-busy tokens
5) execute SETSSBSY to load SSP

The MSRs can be configured whenever, subject to suitable hardware
support.  In both of these cases, we've actually pre-configured the
non-busy supervisor tokens which is why we don't set those up directly. 

Furthermore, we defer setting up MSR_ISST_SSP to when we set up the IDT
and TSS, and that's fine because it doesn't make interrupts/exceptions
any less fatal.

The only hard ordering is that SETSSBSY depends on CR4.CET &&
MSR_S_CET.SHSTK_EN in order to not #UD.

However, between CR4.CET && MSR_S_CET.SHSTK_EN and SETSSBSY, we're
operating with an SSP of 0, meaning that any call/ret/etc are fatal. 
That is why I previously grouped the 3 actions as close to together as
possible.

For the CONFIG_XEN_IBT && !CONFIG_XEN_SHSTK case, we need to set up CR4
and MSR_S_CET only.  This was the only way I could find to lay out the
logic in a half-reasonable way.  It does mean that MSR_PL0_SSP is set up
during the critical call/ret region, but that's the smallest price I
could find to pay.  Anything else would have had more conditionals, and
substantially more #ifdef-ary.


I have put in this:

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index 9178b2e6a039..6a4834f9813a 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -45,6 +45,8 @@ ENTRY(__high_start)
         mov     $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx
         mov     %rcx, %cr4
 
+        /* WARNING! call/ret/etc now fatal (iff SHSTK) until SETSSBSY
loads SSP */
+
 #if defined(CONFIG_XEN_SHSTK)
         test    $CET_SHSTK_EN, %al
         jz      .L_ap_cet_done


which mirrors our Spectre-v2 warning in the entry paths.

~Andrew