Re: [PATCH 63/65] x86/setup: Rework MSR_S_CET handling for CET-IBT
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) >>> +callxen_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
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) >> +callxen_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
Re: [PATCH 63/65] x86/setup: Rework MSR_S_CET handling for CET-IBT
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) > +callxen_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. > +/* 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.) > +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. Jan
[PATCH 63/65] x86/setup: Rework MSR_S_CET handling for CET-IBT
CET-SS and CET-IBT can be independently controlled, so the configuration of MSR_S_CET can't be constants any more. Introduce xen_msr_s_cet_value(), mostly because I don't fancy writing/maintaining that logic in assembly. Use this in the 3 paths which alter MSR_S_CET when both features are potentially active. To active CET-IBT, we only need CR4.CET and MSR_S_CET.ENDBR_EN. This is common with the CET-SS setup, so reorder the operations to set up CR4 and MSR_S_CET for any nonzero result from xen_msr_s_cet_value(), and set up MSR_PL0_SSP and SSP if SHSTK_EN was also set. Adjust the crash path to disable CET-IBT too. Signed-off-by: Andrew Cooper --- CC: Jan Beulich CC: Roger Pau Monné CC: Wei Liu It is quite possible that the S3 path is dead code. CET-IBT only exist on Intel systems from TigerLake onwards, and TGL kills S3 in favour of the newer S0ix power state. AMD Ryzen platforms (Zen3 onwards) support S3 and CET-SS, so partial testing will occur there. --- xen/arch/x86/acpi/wakeup_prot.S | 37 ++--- xen/arch/x86/boot/x86_64.S | 29 ++--- xen/arch/x86/crash.c| 4 ++-- xen/arch/x86/setup.c| 17 - xen/include/asm-x86/msr-index.h | 1 + 5 files changed, 59 insertions(+), 29 deletions(-) diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S index 15052c300fa1..01eb26ed0769 100644 --- 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) +callxen_msr_s_cet_value +test%eax, %eax +je .L_cet_done + +/* 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 + +#if defined(CONFIG_XEN_SHSTK) +test$CET_SHSTK_EN, %eax +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 - /* Write the temporary token onto the shadow stack, and activate it. */ wrssq %rdi, (%rdi) +/* MSR_INTERRUPT_SSP_TABLE is set up later in load_system_tables(). */ setssbsy /* Reset MSR_PL0_SSP back to its normal value. */ @@ -106,8 +112,9 @@ ENTRY(s3_resume) /* Pop the temporary token off the stack. */ mov $2, %eax incsspd %eax -.L_shstk_done: -#endif +#endif /* CONFIG_XEN_SHSTK */ +.L_cet_done: +#endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */ callload_system_tables diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index d61048c583b3..c05c69f9fa59 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -30,18 +30,25 @@ ENTRY(__high_start) test%ebx,%ebx jz .L_bsp -/* APs. Set up shadow stacks before entering C. */ -#ifdef CONFIG_XEN_SHSTK -testl $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \ -CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip) -je .L_ap_shstk_done +/* APs. Set up CET before entering C properly. */ +#if defined(CONFIG_XEN_SHSTK) || defined(CONFIG_XEN_IBT) +callxen_msr_s_cet_value +test%eax, %eax +je .L_ap_cet_done /* Set up MSR_S_CET. */ mov $MSR_S_CET, %ecx xor %edx, %edx -mov $CET_SHSTK_EN | CET_WRSS_EN, %eax wrmsr +/* Enable CR4.CET. */ +mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx +mov %rcx, %cr4 + +#if defined(CONFIG_XEN_SHSTK) +test$CET_SHSTK_EN, %eax +je .L_ap_cet_done + /* Derive MSR_PL0_SSP from %rsp (token written when stack is allocated). */ mov $MSR_PL0_SSP, %ecx mov %rsp, %rdx @@ -51,13 +58,13 @@ ENTRY(__high_start) or $(PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - 8, %eax wrmsr -/* Enable CET.
