[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Andrew Cooper <amc96@xxxxxxxx>, Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Fri, 10 Dec 2021 17:47:17 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=O54XzNz6VIIfsiZ4FsDIUDFLH3VGBKmwPcLZtpPVZuY=; b=F0wnHUBtqLzJlfF1DAHhgs+o2Q0fyEa3W9EwxVsNBRS1dpi0ooW8CKg96D2eeYmTPvSuv7bNoyIqxZtI5c1jnzE8xuGS28GE8m6BdkeJmOo5Fvj1mV01NtgZVX4e9nlfXF7/gq03BLKHyYhBkY/isKIbQKkMKXgrItAaURYqFDEF/MbrqT9+tu7dt4VjXm4tgQ/lpkpg9GWz74iXDy+p1apnKpXnJXFW+mt7U3yM4zLRuCH3iZnMkIXm1KmqZxrNDLgoxvLGQFtrYkzFkkpDqdsFVWIvuKTOlDgfxh0h+ye1CfZZkPipu9j2luRlBarTaarQ9vugcjS6YOFr/WgVEw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=a01D+cUHheCs2ZNGAAMaum/jJfCm0hr/ukjpi36ZCpUpuw7DLGdmW6FWSZW7NpbuFsRTR6ge0zqhEjgvULXD+lhfb0Rl4v4lfVqWLfqBpD06Eg8F11MtJnsUOwpZEnDHlGtgoQHNOONNJi0aQ7wig2Y9fa65I93eriFvuYA31YT6bSw3m2XRWV12THNAVLjmdnu9NxIT4Fn0BxkX6OGotE1WtJm3Zs4bHBlfTF0RT+jLzn6eL4RSB3muzH/jxMJjcg3kfFgAORKYz0FIS3+XLhcGqY8pPrYA55sQdNOojzSUjVY3lZTHDRVcZocje/7cjV5KrR5wKwPLWmy7ShqOhQ==
  • Authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Fri, 10 Dec 2021 16:47:36 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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




 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.