[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
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |