| Message ID | 20220214125127.17985-69-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Support for CET Indirect Branch Tracking | expand |
On 14.02.2022 13:51, Andrew Cooper wrote: > CET-SS and CET-IBT can be independently controlled, so the configuration of > MSR_S_CET can't be constant 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 <andrew.cooper3@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> albeit with a nit and a remark: > --- a/xen/arch/x86/acpi/wakeup_prot.S > +++ b/xen/arch/x86/acpi/wakeup_prot.S > @@ -63,7 +63,26 @@ 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 > + jz .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 > + > + /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads SSP */ > + > +#if defined(CONFIG_XEN_SHSTK) Just #ifdef, as it was before? > @@ -90,10 +101,6 @@ 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 latter part of this comment could do with retaining. Jan
On 15/02/2022 16:46, Jan Beulich wrote: > On 14.02.2022 13:51, Andrew Cooper wrote: >> CET-SS and CET-IBT can be independently controlled, so the configuration of >> MSR_S_CET can't be constant 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 <andrew.cooper3@citrix.com> > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks, > albeit with a nit and a remark: > >> --- a/xen/arch/x86/acpi/wakeup_prot.S >> +++ b/xen/arch/x86/acpi/wakeup_prot.S >> @@ -63,7 +63,26 @@ 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 >> + jz .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 >> + >> + /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads SSP */ >> + >> +#if defined(CONFIG_XEN_SHSTK) > Just #ifdef, as it was before? I can if you insist, but that's breaking consistency with the other ifdefary. > >> @@ -90,10 +101,6 @@ 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 latter part of this comment could do with retaining. So I tried that in v1, and concluded not for v2. There is nowhere appropriate for it to live, anywhere in this block. And it is an artefact of me bootstrapping SHSTK to start with. The truth is that nothing about MSR_ISST_TABLE matters until load_system_table sets up both this and the TSS IST fields together. IST exceptions are already fatal at this point for non-SHSTK reasons. ~Andrew
On 15.02.2022 21:58, Andrew Cooper wrote: > On 15/02/2022 16:46, Jan Beulich wrote: >> On 14.02.2022 13:51, Andrew Cooper wrote: >>> CET-SS and CET-IBT can be independently controlled, so the configuration of >>> MSR_S_CET can't be constant 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 <andrew.cooper3@citrix.com> >> Reviewed-by: Jan Beulich <jbeulich@suse.com> > > Thanks, > >> albeit with a nit and a remark: >> >>> --- a/xen/arch/x86/acpi/wakeup_prot.S >>> +++ b/xen/arch/x86/acpi/wakeup_prot.S >>> @@ -63,7 +63,26 @@ 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 >>> + jz .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 >>> + >>> + /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads SSP */ >>> + >>> +#if defined(CONFIG_XEN_SHSTK) >> Just #ifdef, as it was before? > > I can if you insist, but that's breaking consistency with the other > ifdefary. I guess consistent of not depends on the way you look at it. I generally think simple conditionals should just use #ifdef. As soon as there's an #elif or a more complex condition, #if defined() is of course more consistent. But one #ifdef nested inside another #if imo isn't a reason to use #if in both places. Nevertheless, ftaod - I'm not going to insist, as I can see this being a matter of personal preference. >>> @@ -90,10 +101,6 @@ 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 latter part of this comment could do with retaining. > > So I tried that in v1, and concluded not for v2. > > There is nowhere appropriate for it to live, anywhere in this block. > And it is an artefact of me bootstrapping SHSTK to start with. > > The truth is that nothing about MSR_ISST_TABLE matters until > load_system_table sets up both this and the TSS IST fields together. > IST exceptions are already fatal at this point for non-SHSTK reasons. Well, okay. To me, not being as familiar with this code as you are, the comments was quite helpful ... Jan
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S index 15052c300fa1..3855ff1ddb94 100644 --- a/xen/arch/x86/acpi/wakeup_prot.S +++ b/xen/arch/x86/acpi/wakeup_prot.S @@ -63,7 +63,26 @@ 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 + jz .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 + + /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads SSP */ + +#if defined(CONFIG_XEN_SHSTK) + test $CET_SHSTK_EN, %al + jz .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 +90,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,10 +101,6 @@ 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) setssbsy @@ -106,8 +113,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 */ call load_system_tables diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 27f52e7a7708..fa41990dde0f 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -30,18 +30,27 @@ 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) + call xen_msr_s_cet_value + test %eax, %eax + jz .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 + + /* WARNING! call/ret now fatal (iff SHSTK) until SETSSBSY loads SSP */ + +#if defined(CONFIG_XEN_SHSTK) + test $CET_SHSTK_EN, %al + jz .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 +60,12 @@ ENTRY(__high_start) or $(PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - 8, %eax wrmsr - /* Enable CET. MSR_INTERRUPT_SSP_TABLE is set up later in load_system_tables(). */ - mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx - mov %rcx, %cr4 setssbsy -#endif -.L_ap_shstk_done: +#endif /* CONFIG_XEN_SHSTK */ +.L_ap_cet_done: +#endif /* CONFIG_XEN_SHSTK || CONFIG_XEN_IBT */ + call start_secondary BUG /* start_secondary() shouldn't return. */ diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c index c383f718f5bd..003222c0f1ac 100644 --- a/xen/arch/x86/crash.c +++ b/xen/arch/x86/crash.c @@ -190,8 +190,8 @@ void machine_crash_shutdown(void) /* Reset CPUID masking and faulting to the host's default. */ ctxt_switch_levelling(NULL); - /* Disable shadow stacks. */ - if ( cpu_has_xen_shstk ) + /* Disable CET. */ + if ( cpu_has_xen_shstk || cpu_has_xen_ibt ) { wrmsrl(MSR_S_CET, 0); write_cr4(read_cr4() & ~X86_CR4_CET); diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h index 9df1959fe5a1..3e038db618ff 100644 --- a/xen/arch/x86/include/asm/msr-index.h +++ b/xen/arch/x86/include/asm/msr-index.h @@ -117,6 +117,7 @@ #define MSR_S_CET 0x000006a2 #define CET_SHSTK_EN (_AC(1, ULL) << 0) #define CET_WRSS_EN (_AC(1, ULL) << 1) +#define CET_ENDBR_EN (_AC(1, ULL) << 2) #define MSR_PL0_SSP 0x000006a4 #define MSR_PL1_SSP 0x000006a5 diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index 2b1192d85b77..f6a59d5f0412 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -670,6 +670,21 @@ static void noreturn init_done(void) startup_cpu_idle_loop(); } +#if defined(CONFIG_XEN_SHSTK) || defined(CONFIG_XEN_IBT) +/* + * Used by AP and S3 asm code to calcualte the appropriate MSR_S_CET setting. + * Do not use on the BSP before reinit_bsp_stack(), or it may turn SHSTK on + * too early. + */ +unsigned int xen_msr_s_cet_value(void) +{ + return ((cpu_has_xen_shstk ? CET_SHSTK_EN | CET_WRSS_EN : 0) | + (cpu_has_xen_ibt ? CET_ENDBR_EN : 0)); +} +#else +unsigned int xen_msr_s_cet_value(void); /* To avoid ifdefary */ +#endif + /* Reinitalise all state referring to the old virtual address of the stack. */ static void __init noreturn reinit_bsp_stack(void) { @@ -693,7 +708,7 @@ static void __init noreturn reinit_bsp_stack(void) { wrmsrl(MSR_PL0_SSP, (unsigned long)stack + (PRIMARY_SHSTK_SLOT + 1) * PAGE_SIZE - 8); - wrmsrl(MSR_S_CET, CET_SHSTK_EN | CET_WRSS_EN); + wrmsrl(MSR_S_CET, xen_msr_s_cet_value()); asm volatile ("setssbsy" ::: "memory"); }
CET-SS and CET-IBT can be independently controlled, so the configuration of MSR_S_CET can't be constant 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 <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Wei Liu <wl@xen.org> v2: * Asm adjustments. Add comments regarding safety. --- xen/arch/x86/acpi/wakeup_prot.S | 38 ++++++++++++++++++++++-------------- xen/arch/x86/boot/x86_64.S | 30 +++++++++++++++++----------- xen/arch/x86/crash.c | 4 ++-- xen/arch/x86/include/asm/msr-index.h | 1 + xen/arch/x86/setup.c | 17 +++++++++++++++- 5 files changed, 61 insertions(+), 29 deletions(-)