|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH 2/2] x86/shskt: Disable CET-SS on parts succeptable to fractured updates
On Sat, Dec 31, 2022 at 12:30:07AM +0000, Andrew Cooper wrote:
> Refer to Intel SDM Rev 70 (Dec 2022), Vol3 17.2.3 "Supervisor Shadow Stack
> Token".
>
> Architecturally, an event delivery which starts in CPL>3 and switches shadow
> stack will first validate the Supervisor Shstk Token and set the busy bit,
> then pushes LIP/CS/SSP. One example of this is an NMI interrupting Xen.
>
> Some CPUs suffer from an issue called fracturing, whereby a fault/vmexit/etc
> between setting the busy bit and completing the event injection renders the
> action non-restartable, because when it comes time to restart, the busy bit is
> found to be already set.
>
> This is far more easily encountered under virt, yet it is not the fault of the
> hypervisor, nor the fault of the guest kernel. The fault lies somewhere
> between the architectural specification, and the uarch behaviour.
>
> Intel have allocated CPUID.7[1].ecx[18] CET_SSS to enumerate that supervisor
> shadow stacks are safe to use. Because of how Xen lays out its shadow stacks,
> fracturing is not expected to be a problem on native.
>
> Detect this case on boot and default to not using shstk if virtualised.
> Specifying `cet=shstk` on the command line will override this heurstic and
> enable shadow stacks irrespective.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
>
> I've got a query out with AMD, but so far it is only Intel CPUs known to be
> impacted.
>
> This ideally wants backporting to Xen 4.14. I have no idea how likely it is
> to need to backport the prerequisite patch for new feature words, but we've
> already had to do that once for security patches...
> ---
> docs/misc/xen-command-line.pandoc | 7 +++++-
> tools/libs/light/libxl_cpuid.c | 2 ++
> tools/misc/xen-cpuid.c | 1 +
> xen/arch/x86/cpu/common.c | 11 +++++++--
> xen/arch/x86/setup.c | 37
> ++++++++++++++++++++++++++---
> xen/include/public/arch-x86/cpufeatureset.h | 1 +
> 6 files changed, 53 insertions(+), 6 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc
> b/docs/misc/xen-command-line.pandoc
> index 923910f553c5..19d4d815bdee 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -287,10 +287,15 @@ can be maintained with the pv-shim mechanism.
> protection.
>
> The option is available when `CONFIG_XEN_SHSTK` is compiled in, and
> - defaults to `true` on hardware supporting CET-SS. Specifying
> + generally defaults to `true` on hardware supporting CET-SS. Specifying
> `cet=no-shstk` will cause Xen not to use Shadow Stacks even when support
> is available in hardware.
>
> + Some hardware suffers from an issue known as Supervisor Shadow Stack
> + Fracturing. On such hardware, Xen will default to not using Shadow
> Stacks
> + when virtualised. Specifying `cet=shstk` will override this heuristic
> and
> + enable Shadow Stacks unilaterally.
> +
> * The `ibt=` boolean controls whether Xen uses Indirect Branch Tracking for
> its own protection.
>
> diff --git a/tools/libs/light/libxl_cpuid.c b/tools/libs/light/libxl_cpuid.c
> index 2aa23225f42c..d97a2f3338bc 100644
> --- a/tools/libs/light/libxl_cpuid.c
> +++ b/tools/libs/light/libxl_cpuid.c
> @@ -235,6 +235,8 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list
> *cpuid, const char* str)
> {"fsrs", 0x00000007, 1, CPUID_REG_EAX, 11, 1},
> {"fsrcs", 0x00000007, 1, CPUID_REG_EAX, 12, 1},
>
> + {"cet-sss", 0x00000007, 1, CPUID_REG_EDX, 18, 1},
> +
> {"intel-psfd", 0x00000007, 2, CPUID_REG_EDX, 0, 1},
> {"mcdt-no", 0x00000007, 2, CPUID_REG_EDX, 5, 1},
>
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index 0091a11a67bc..ea33b587665d 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -208,6 +208,7 @@ static const char *const str_7c1[32] =
>
> static const char *const str_7d1[32] =
> {
> + [18] = "cet-sss",
> };
>
> static const char *const str_7d2[32] =
> diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
> index b3fcf4680f3a..d962f384a995 100644
> --- a/xen/arch/x86/cpu/common.c
> +++ b/xen/arch/x86/cpu/common.c
> @@ -346,11 +346,18 @@ 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,
> + if (c->cpuid_level >= 7) {
> + uint32_t max_subleaf;
> +
> + cpuid_count(7, 0, &max_subleaf, &ebx,
> &c->x86_capability[FEATURESET_7c0],
> &c->x86_capability[FEATURESET_7d0]);
>
> + if (max_subleaf >= 1)
tabs vs spaces ...
Is this file imported from Linux? It uses tabs for indentation, contrary
to the rest of the Xen code base.
> + cpuid_count(7, 1, &eax, &ebx, &ecx,
> + &c->x86_capability[FEATURESET_7d1]);
> + }
> +
> eax = cpuid_eax(0x80000000);
> if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
> ebx = eax >= 0x8000001f ? cpuid_ebx(0x8000001f) : 0;
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 566422600d94..e052b7b748fa 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -96,7 +96,7 @@ size_param("highmem-start", highmem_start);
> #endif
>
> #ifdef CONFIG_XEN_SHSTK
> -static bool __initdata opt_xen_shstk = true;
> +static int8_t __initdata opt_xen_shstk = -1;
> #else
> #define opt_xen_shstk false
> #endif
> @@ -1101,9 +1101,40 @@ void __init noreturn __start_xen(unsigned long mbi_p)
> /* 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");
> + /*
> + * Some CPUs suffer from Shadow Stack Fracturing, an issue whereby a
> + * fault/VMExit/etc between setting a Supervisor Busy bit and the
> + * event delivery completing renders the operation non-restartable.
> + * On restart, event delivery will find the Busy bit already set.
> + *
> + * This is a problem on native, but outside of synthetic cases, only
> + * with #MC against a stack access (in which case we're dead anyway).
> + * It is a much bigger problem under virt, because we can VMExit for
> a
> + * number of legitimate reasons and tickle this bug.
> + *
> + * CPUs with this addressed enumerate CET-SSS to indicate that
> + * supervisor shadow stacks are now safe to use.
> + */
> + bool cpu_has_bug_shstk_fracture =
> + boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> + !boot_cpu_has(X86_FEATURE_CET_SSS);
> +
> + /*
> + * On native, assume that Xen won't be impacted by shstk fracturing
> + * problems. Under virt, be more conservative and disable shstk by
> + * default.
> + */
> + if ( opt_xen_shstk == -1 )
> + opt_xen_shstk =
> + cpu_has_hypervisor ? !cpu_has_bug_shstk_fracture
> + : true;
> +
> + if ( opt_xen_shstk )
> + {
> + printk("Enabling Supervisor Shadow Stacks\n");
>
> - setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK);
> + setup_force_cpu_cap(X86_FEATURE_XEN_SHSTK);
> + }
> }
>
> if ( opt_xen_ibt && boot_cpu_has(X86_FEATURE_CET_IBT) )
> diff --git a/xen/include/public/arch-x86/cpufeatureset.h
> b/xen/include/public/arch-x86/cpufeatureset.h
> index 7a896f0e2d92..f6a46f62a549 100644
> --- a/xen/include/public/arch-x86/cpufeatureset.h
> +++ b/xen/include/public/arch-x86/cpufeatureset.h
> @@ -290,6 +290,7 @@ XEN_CPUFEATURE(INTEL_PPIN, 12*32+ 0) /*
> Protected Processor Inventory
>
> /* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */
> /* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */
> +XEN_CPUFEATURE(CET_SSS, 15*32+18) /* CET Supervisor Shadow
> Stacks safe to use */
>
> /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */
> XEN_CPUFEATURE(INTEL_PSFD, 13*32+ 0) /*A MSR_SPEC_CTRL.PSFD */
> --
> 2.11.0
>
>
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
Attachment:
signature.asc
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |