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

[Jan Beulich <jbeulich@suse.com>]

On 10.12.2021 17:19, Andrew Cooper wrote:
> 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.

Thanks, I think this may end up helpful down the road.

Jan