| Message ID | 20200501225838.9866-17-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
On 02.05.2020 00:58, Andrew Cooper wrote: > --- a/xen/arch/x86/acpi/wakeup_prot.S > +++ b/xen/arch/x86/acpi/wakeup_prot.S > @@ -1,3 +1,8 @@ > +#include <asm/config.h> Why is this needed? Afaics assembly files, just like C ones, get xen/config.h included from the compiler command line. > @@ -48,6 +59,48 @@ ENTRY(s3_resume) > pushq %rax > lretq > 1: > +#ifdef CONFIG_XEN_SHSTK > + /* > + * Restoring SSP is a little convoluted, because we are intercepting > + * the middle of an in-use shadow stack. Write a temporary supervisor > + * token under the stack, so SETSSBSY takes us where we want, then > + * reset MSR_PL0_SSP to its usual value and pop the temporary token. What do you mean by "takes us where we want"? I take it "us" is really SSP here? > + */ > + mov saved_rsp(%rip), %rdi > + cmpq $1, %rdi > + je .L_shstk_done > + > + /* Write a supervisor token under SSP. */ > + sub $8, %rdi > + mov %rdi, (%rdi) > + > + /* Load it into MSR_PL0_SSP. */ > + mov $MSR_PL0_SSP, %ecx > + mov %rdi, %rdx > + shr $32, %rdx > + mov %edi, %eax > + > + /* Enable CET. */ > + mov $MSR_S_CET, %ecx > + xor %edx, %edx > + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax > + wrmsr > + > + /* Activate our temporary token. */ > + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx > + mov %rbx, %cr4 > + setssbsy > + > + /* Reset MSR_PL0_SSP back to its expected value. */ > + and $~(STACK_SIZE - 1), %eax > + or $0x5ff8, %eax > + wrmsr Ahead of this WRMSR neither %ecx nor %edx look to have their intended values anymore. Also there is a again a magic 0x5ff8 here (and at least one more further down). > --- a/xen/arch/x86/boot/x86_64.S > +++ b/xen/arch/x86/boot/x86_64.S > @@ -28,8 +28,36 @@ ENTRY(__high_start) > lretq > 1: > test %ebx,%ebx > - jnz start_secondary > + jz .L_bsp > > + /* APs. Set up shadow stacks before entering C. */ > + > + testl $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \ > + CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip) > + je .L_ap_shstk_done > + > + mov $MSR_S_CET, %ecx > + xor %edx, %edx > + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax > + wrmsr > + > + mov $MSR_PL0_SSP, %ecx > + mov %rsp, %rdx > + shr $32, %rdx > + mov %esp, %eax > + and $~(STACK_SIZE - 1), %eax > + or $0x5ff8, %eax > + wrmsr > + > + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx > + mov %rcx, %cr4 > + setssbsy Since the token doesn't get written here, could you make the comment say where this happens? I have to admit that I had to go through earlier patches to find it again. > +.L_ap_shstk_done: > + call start_secondary > + BUG /* start_secondary() shouldn't return. */ This conversion from a jump to CALL is unrelated and hence would better be mentioned in the description imo. > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -323,6 +323,11 @@ void __init early_cpu_init(void) > x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86, > c->x86_model, c->x86_model, c->x86_mask, eax); > > + if (c->cpuid_level >= 7) { > + cpuid_count(7, 0, &eax, &ebx, &ecx, &edx); > + c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx; > + } How about moving the leaf 7 code from generic_identify() here as a whole? > --- a/xen/arch/x86/setup.c > +++ b/xen/arch/x86/setup.c > @@ -664,6 +664,13 @@ static void __init noreturn reinit_bsp_stack(void) > stack_base[0] = stack; > memguard_guard_stack(stack); > > + if ( cpu_has_xen_shstk ) > + { > + wrmsrl(MSR_PL0_SSP, (unsigned long)stack + 0x5ff8); > + wrmsrl(MSR_S_CET, CET_SHSTK_EN | CET_WRSS_EN); > + asm volatile ("setssbsy" ::: "memory"); > + } Same as for APs - a brief comment pointing at where the token was written would seem helpful. Could you also have the patch description say a word on the choice of enabling CET_WRSS_EN uniformly and globally? > @@ -985,6 +992,21 @@ void __init noreturn __start_xen(unsigned long mbi_p) > /* This must come before e820 code because it sets paddr_bits. */ > early_cpu_init(); > > + /* Choose shadow stack early, to set infrastructure up appropriately. */ > + if ( opt_xen_shstk && boot_cpu_has(X86_FEATURE_CET_SS) ) > + { > + printk("Enabling Supervisor Shadow Stacks\n"); > + > + setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK); > +#ifdef CONFIG_PV32 > + if ( opt_pv32 ) > + { > + opt_pv32 = 0; > + printk(" - Disabling PV32 due to Shadow Stacks\n"); > + } > +#endif I think this deserves an explanation, either in a comment or in the patch description. > @@ -1721,6 +1743,10 @@ void __init noreturn __start_xen(unsigned long mbi_p) > > alternative_branches(); > > + /* Defer CR4.CET until alternatives have finished playing with CR4.WP */ > + if ( cpu_has_xen_shstk ) > + set_in_cr4(X86_CR4_CET); Nit: CR0.WP (in the comment) Jan
On 07/05/2020 15:54, Jan Beulich wrote: > On 02.05.2020 00:58, Andrew Cooper wrote: >> --- a/xen/arch/x86/acpi/wakeup_prot.S >> +++ b/xen/arch/x86/acpi/wakeup_prot.S >> @@ -1,3 +1,8 @@ >> +#include <asm/config.h> > Why is this needed? Afaics assembly files, just like C ones, get > xen/config.h included from the compiler command line. I'll double check, but I do recall it being necessary. >> @@ -48,6 +59,48 @@ ENTRY(s3_resume) >> pushq %rax >> lretq >> 1: >> +#ifdef CONFIG_XEN_SHSTK >> + /* >> + * Restoring SSP is a little convoluted, because we are intercepting >> + * the middle of an in-use shadow stack. Write a temporary supervisor >> + * token under the stack, so SETSSBSY takes us where we want, then >> + * reset MSR_PL0_SSP to its usual value and pop the temporary token. > What do you mean by "takes us where we want"? I take it "us" is really > SSP here? Load the SSP that we want. SETSSBSY is the only instruction which can do a fairly arbitrary load of SSP, but it still has to complete the check and activation of the supervisor token. >> + */ >> + mov saved_rsp(%rip), %rdi >> + cmpq $1, %rdi >> + je .L_shstk_done >> + >> + /* Write a supervisor token under SSP. */ >> + sub $8, %rdi >> + mov %rdi, (%rdi) >> + >> + /* Load it into MSR_PL0_SSP. */ >> + mov $MSR_PL0_SSP, %ecx >> + mov %rdi, %rdx >> + shr $32, %rdx >> + mov %edi, %eax >> + >> + /* Enable CET. */ >> + mov $MSR_S_CET, %ecx >> + xor %edx, %edx >> + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax >> + wrmsr >> + >> + /* Activate our temporary token. */ >> + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx >> + mov %rbx, %cr4 >> + setssbsy >> + >> + /* Reset MSR_PL0_SSP back to its expected value. */ >> + and $~(STACK_SIZE - 1), %eax >> + or $0x5ff8, %eax >> + wrmsr > Ahead of this WRMSR neither %ecx nor %edx look to have their intended > values anymore. Also there is a again a magic 0x5ff8 here (and at > least one more further down). There is another bug in this version which I spotted and fixed. The write of the supervisor shadow stack token has to be done after CET is enabled and with WRSSQ because the mapping is already read-only. >> --- a/xen/arch/x86/boot/x86_64.S >> +++ b/xen/arch/x86/boot/x86_64.S >> @@ -28,8 +28,36 @@ ENTRY(__high_start) >> lretq >> 1: >> test %ebx,%ebx >> - jnz start_secondary >> + jz .L_bsp >> >> + /* APs. Set up shadow stacks before entering C. */ >> + >> + testl $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \ >> + CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip) >> + je .L_ap_shstk_done >> + >> + mov $MSR_S_CET, %ecx >> + xor %edx, %edx >> + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax >> + wrmsr >> + >> + mov $MSR_PL0_SSP, %ecx >> + mov %rsp, %rdx >> + shr $32, %rdx >> + mov %esp, %eax >> + and $~(STACK_SIZE - 1), %eax >> + or $0x5ff8, %eax >> + wrmsr >> + >> + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx >> + mov %rcx, %cr4 >> + setssbsy > Since the token doesn't get written here, could you make the comment > say where this happens? I have to admit that I had to go through > earlier patches to find it again. Ok. > >> +.L_ap_shstk_done: >> + call start_secondary >> + BUG /* start_secondary() shouldn't return. */ > This conversion from a jump to CALL is unrelated and hence would > better be mentioned in the description imo. > >> --- a/xen/arch/x86/cpu/common.c >> +++ b/xen/arch/x86/cpu/common.c >> @@ -323,6 +323,11 @@ void __init early_cpu_init(void) >> x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86, >> c->x86_model, c->x86_model, c->x86_mask, eax); >> >> + if (c->cpuid_level >= 7) { >> + cpuid_count(7, 0, &eax, &ebx, &ecx, &edx); >> + c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx; >> + } > How about moving the leaf 7 code from generic_identify() here as > a whole? In the past, we've deliberately not done that to avoid code gaining a reliance on the pre-cached values. I have a plan to rework this substantially when I move microcode loading to the start of __start_xen(), at which point early_cpu_init() will disappear and become the BSP's regular cpu_init(). Until then, we shouldn't cache unnecessary leaves this early. >> --- a/xen/arch/x86/setup.c >> +++ b/xen/arch/x86/setup.c >> @@ -664,6 +664,13 @@ static void __init noreturn reinit_bsp_stack(void) >> stack_base[0] = stack; >> memguard_guard_stack(stack); >> >> + if ( cpu_has_xen_shstk ) >> + { >> + wrmsrl(MSR_PL0_SSP, (unsigned long)stack + 0x5ff8); >> + wrmsrl(MSR_S_CET, CET_SHSTK_EN | CET_WRSS_EN); >> + asm volatile ("setssbsy" ::: "memory"); >> + } > Same as for APs - a brief comment pointing at where the token was > written would seem helpful. > > Could you also have the patch description say a word on the choice > of enabling CET_WRSS_EN uniformly and globally? That is an area for possible improvement. For now, it is unilaterally enabled for simplicity. None of the places we need to use WRSSQ are fastpaths. It is in the extable recovery, S3 path and enable_nmi()'s. We can get away with a RDMSR/WRMSR/WRMSR sequence, which keeps us safe to ROP gadgets and problems from poisoning a read-mostly default. >> @@ -985,6 +992,21 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> /* This must come before e820 code because it sets paddr_bits. */ >> early_cpu_init(); >> >> + /* Choose shadow stack early, to set infrastructure up appropriately. */ >> + if ( opt_xen_shstk && boot_cpu_has(X86_FEATURE_CET_SS) ) >> + { >> + printk("Enabling Supervisor Shadow Stacks\n"); >> + >> + setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK); >> +#ifdef CONFIG_PV32 >> + if ( opt_pv32 ) >> + { >> + opt_pv32 = 0; >> + printk(" - Disabling PV32 due to Shadow Stacks\n"); >> + } >> +#endif > I think this deserves an explanation, either in a comment or in > the patch description. Probably both. > >> @@ -1721,6 +1743,10 @@ void __init noreturn __start_xen(unsigned long mbi_p) >> >> alternative_branches(); >> >> + /* Defer CR4.CET until alternatives have finished playing with CR4.WP */ >> + if ( cpu_has_xen_shstk ) >> + set_in_cr4(X86_CR4_CET); > Nit: CR0.WP (in the comment) Oops. ~Andrew
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S index 4dba6020a7..22c0f8cc79 100644 --- a/xen/arch/x86/acpi/wakeup_prot.S +++ b/xen/arch/x86/acpi/wakeup_prot.S @@ -1,3 +1,8 @@ +#include <asm/config.h> +#include <asm/msr-index.h> +#include <asm/page.h> +#include <asm/processor.h> + .file __FILE__ .text .code64 @@ -15,6 +20,12 @@ ENTRY(do_suspend_lowlevel) mov %cr0, %rax mov %rax, saved_cr0(%rip) +#ifdef CONFIG_XEN_SHSTK + mov $1, %eax + rdsspq %rax + mov %rax, saved_ssp(%rip) +#endif + /* enter sleep state physically */ mov $3, %edi call acpi_enter_sleep_state @@ -48,6 +59,48 @@ ENTRY(s3_resume) pushq %rax lretq 1: +#ifdef CONFIG_XEN_SHSTK + /* + * Restoring SSP is a little convoluted, because we are intercepting + * the middle of an in-use shadow stack. Write a temporary supervisor + * token under the stack, so SETSSBSY takes us where we want, then + * reset MSR_PL0_SSP to its usual value and pop the temporary token. + */ + mov saved_rsp(%rip), %rdi + cmpq $1, %rdi + je .L_shstk_done + + /* Write a supervisor token under SSP. */ + sub $8, %rdi + mov %rdi, (%rdi) + + /* Load it into MSR_PL0_SSP. */ + mov $MSR_PL0_SSP, %ecx + mov %rdi, %rdx + shr $32, %rdx + mov %edi, %eax + + /* Enable CET. */ + mov $MSR_S_CET, %ecx + xor %edx, %edx + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax + wrmsr + + /* Activate our temporary token. */ + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ebx + mov %rbx, %cr4 + setssbsy + + /* Reset MSR_PL0_SSP back to its expected value. */ + and $~(STACK_SIZE - 1), %eax + or $0x5ff8, %eax + wrmsr + + /* Pop the temporary token off the stack. */ + mov $2, %eax + incsspd %eax +.L_shstk_done: +#endif call load_system_tables @@ -65,6 +118,9 @@ ENTRY(s3_resume) saved_rsp: .quad 0 saved_cr0: .quad 0 +#ifdef CONFIG_XEN_SHSTK +saved_ssp: .quad 0 +#endif GLOBAL(saved_magic) .long 0x9abcdef0 diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S index 314a32a19f..59b770f955 100644 --- a/xen/arch/x86/boot/x86_64.S +++ b/xen/arch/x86/boot/x86_64.S @@ -28,8 +28,36 @@ ENTRY(__high_start) lretq 1: test %ebx,%ebx - jnz start_secondary + jz .L_bsp + /* APs. Set up shadow stacks before entering C. */ + + testl $cpufeat_mask(X86_FEATURE_XEN_SHSTK), \ + CPUINFO_FEATURE_OFFSET(X86_FEATURE_XEN_SHSTK) + boot_cpu_data(%rip) + je .L_ap_shstk_done + + mov $MSR_S_CET, %ecx + xor %edx, %edx + mov $CET_SHSTK_EN | CET_WRSS_EN, %eax + wrmsr + + mov $MSR_PL0_SSP, %ecx + mov %rsp, %rdx + shr $32, %rdx + mov %esp, %eax + and $~(STACK_SIZE - 1), %eax + or $0x5ff8, %eax + wrmsr + + mov $XEN_MINIMAL_CR4 | X86_CR4_CET, %ecx + mov %rcx, %cr4 + setssbsy + +.L_ap_shstk_done: + call start_secondary + BUG /* start_secondary() shouldn't return. */ + +.L_bsp: /* Pass off the Multiboot info structure to C land (if applicable). */ mov multiboot_ptr(%rip),%edi call __start_xen diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 3962717aa5..a77be36349 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -323,6 +323,11 @@ void __init early_cpu_init(void) x86_cpuid_vendor_to_str(c->x86_vendor), c->x86, c->x86, c->x86_model, c->x86_model, c->x86_mask, eax); + if (c->cpuid_level >= 7) { + cpuid_count(7, 0, &eax, &ebx, &ecx, &edx); + c->x86_capability[cpufeat_word(X86_FEATURE_CET_SS)] = ecx; + } + eax = cpuid_eax(0x80000000); if ((eax >> 16) == 0x8000 && eax >= 0x80000008) { eax = cpuid_eax(0x80000008); diff --git a/xen/arch/x86/crash.c b/xen/arch/x86/crash.c index 450eecd46b..0611b4fb9b 100644 --- a/xen/arch/x86/crash.c +++ b/xen/arch/x86/crash.c @@ -200,6 +200,13 @@ 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 ) + { + wrmsrl(MSR_S_CET, 0); + write_cr4(read_cr4() & ~X86_CR4_CET); + } + info = kexec_crash_save_info(); info->xen_phys_start = xen_phys_start; info->dom0_pfn_to_mfn_frame_list_list = diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c index aa21201507..5c574b2035 100644 --- a/xen/arch/x86/setup.c +++ b/xen/arch/x86/setup.c @@ -664,6 +664,13 @@ static void __init noreturn reinit_bsp_stack(void) stack_base[0] = stack; memguard_guard_stack(stack); + if ( cpu_has_xen_shstk ) + { + wrmsrl(MSR_PL0_SSP, (unsigned long)stack + 0x5ff8); + wrmsrl(MSR_S_CET, CET_SHSTK_EN | CET_WRSS_EN); + asm volatile ("setssbsy" ::: "memory"); + } + reset_stack_and_jump_nolp(init_done); } @@ -985,6 +992,21 @@ void __init noreturn __start_xen(unsigned long mbi_p) /* This must come before e820 code because it sets paddr_bits. */ early_cpu_init(); + /* Choose shadow stack early, to set infrastructure up appropriately. */ + if ( opt_xen_shstk && boot_cpu_has(X86_FEATURE_CET_SS) ) + { + printk("Enabling Supervisor Shadow Stacks\n"); + + setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK); +#ifdef CONFIG_PV32 + if ( opt_pv32 ) + { + opt_pv32 = 0; + printk(" - Disabling PV32 due to Shadow Stacks\n"); + } +#endif + } + /* Sanitise the raw E820 map to produce a final clean version. */ max_page = raw_max_page = init_e820(memmap_type, &e820_raw); @@ -1721,6 +1743,10 @@ void __init noreturn __start_xen(unsigned long mbi_p) alternative_branches(); + /* Defer CR4.CET until alternatives have finished playing with CR4.WP */ + if ( cpu_has_xen_shstk ) + set_in_cr4(X86_CR4_CET); + /* * NB: when running as a PV shim VCPUOP_up/down is wired to the shim * physical cpu_add/remove functions, so launch the guest with only diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c index c5d8e587a8..a94be2d594 100644 --- a/xen/arch/x86/spec_ctrl.c +++ b/xen/arch/x86/spec_ctrl.c @@ -882,6 +882,14 @@ void __init init_speculation_mitigations(void) hw_smt_enabled = check_smt_enabled(); /* + * First, disable the use of retpolines if Xen is using shadow stacks, as + * they are incompatible. + */ + if ( cpu_has_xen_shstk && + (opt_thunk == THUNK_DEFAULT || opt_thunk == THUNK_RETPOLINE) ) + thunk = THUNK_JMP; + + /* * Has the user specified any custom BTI mitigations? If so, follow their * instructions exactly and disable all heuristics. */ diff --git a/xen/include/asm-x86/msr-index.h b/xen/include/asm-x86/msr-index.h index 85c5f20b76..cdfb7b047b 100644 --- a/xen/include/asm-x86/msr-index.h +++ b/xen/include/asm-x86/msr-index.h @@ -68,6 +68,9 @@ #define MSR_U_CET 0x000006a0 #define MSR_S_CET 0x000006a2 +#define CET_SHSTK_EN (_AC(1, ULL) << 0) +#define CET_WRSS_EN (_AC(1, ULL) << 1) + #define MSR_PL0_SSP 0x000006a4 #define MSR_PL1_SSP 0x000006a5 #define MSR_PL2_SSP 0x000006a6 diff --git a/xen/include/asm-x86/x86-defns.h b/xen/include/asm-x86/x86-defns.h index 84e15b15be..4051a80485 100644 --- a/xen/include/asm-x86/x86-defns.h +++ b/xen/include/asm-x86/x86-defns.h @@ -73,6 +73,7 @@ #define X86_CR4_SMEP 0x00100000 /* enable SMEP */ #define X86_CR4_SMAP 0x00200000 /* enable SMAP */ #define X86_CR4_PKE 0x00400000 /* enable PKE */ +#define X86_CR4_CET 0x00800000 /* Control-flow Enforcement Technology */ /* * XSTATE component flags in XCR0
With all other plumbing in place, activate shadow stacks when possible. The BSP needs to wait until alternatives have run (to avoid interaction with CR0.WP), and after the first reset_stack_and_jump() to avoid having a pristine shadow stack interact in problematic ways with an in-use regular stack. Activate shadow stack in reinit_bsp_stack(). APs have all infrastructure set up by the booting CPU, so enable shadow stacks before entering C. The S3 path needs save and restore SSP along side RSP. The crash path needs to turn CET off to avoid interfereing with the kexec kernel's environment. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/acpi/wakeup_prot.S | 56 +++++++++++++++++++++++++++++++++++++++++ xen/arch/x86/boot/x86_64.S | 30 +++++++++++++++++++++- xen/arch/x86/cpu/common.c | 5 ++++ xen/arch/x86/crash.c | 7 ++++++ xen/arch/x86/setup.c | 26 +++++++++++++++++++ xen/arch/x86/spec_ctrl.c | 8 ++++++ xen/include/asm-x86/msr-index.h | 3 +++ xen/include/asm-x86/x86-defns.h | 1 + 8 files changed, 135 insertions(+), 1 deletion(-)