[PATCH 0/2] x86: Work around Shstk fracturing

[PATCH 0/2] x86: Work around Shstk fracturing

[Andrew Cooper]

See patch 2 for details.

Andrew Cooper (2):
  x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}
  x86/shskt: Disable CET-SS on parts succeptable to fractured updates

 docs/misc/xen-command-line.pandoc           |  7 +++++-
 tools/libs/light/libxl_cpuid.c              |  2 ++
 tools/misc/xen-cpuid.c                      | 11 +++++++++
 xen/arch/x86/cpu/common.c                   | 14 ++++++++---
 xen/arch/x86/setup.c                        | 37 ++++++++++++++++++++++++++---
 xen/include/public/arch-x86/cpufeatureset.h |  4 ++++
 xen/include/xen/lib/x86/cpuid.h             | 15 +++++++++++-
 7 files changed, 82 insertions(+), 8 deletions(-)

-- 
2.11.0

[PATCH 1/2] x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}

[Andrew Cooper]

We don't actually need ecx yet, but adding it in now will reduce the amount to
which leaf 7 is out of order in a featureset.

cpufeatureset.h remains in leaf architectrual order for the sanity of anyone
trying to locate where to insert new rows.

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>
---
 tools/misc/xen-cpuid.c                      | 10 ++++++++++
 xen/arch/x86/cpu/common.c                   |  3 ++-
 xen/include/public/arch-x86/cpufeatureset.h |  3 +++
 xen/include/xen/lib/x86/cpuid.h             | 15 ++++++++++++++-
 4 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
index d5833e9ce879..0091a11a67bc 100644
--- a/tools/misc/xen-cpuid.c
+++ b/tools/misc/xen-cpuid.c
@@ -202,6 +202,14 @@ static const char *const str_7b1[32] =
     [ 0] = "ppin",
 };
 
+static const char *const str_7c1[32] =
+{
+};
+
+static const char *const str_7d1[32] =
+{
+};
+
 static const char *const str_7d2[32] =
 {
     [ 0] = "intel-psfd",
@@ -229,6 +237,8 @@ static const struct {
     { "0x80000021.eax",  "e21a", str_e21a },
     { "0x00000007:1.ebx", "7b1", str_7b1 },
     { "0x00000007:2.edx", "7d2", str_7d2 },
+    { "0x00000007:1.ecx", "7b1", str_7c1 },
+    { "0x00000007:1.edx", "7b1", str_7d1 },
 };
 
 #define COL_ALIGN "18"
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index 0412dbc915e5..b3fcf4680f3a 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -450,7 +450,8 @@ static void generic_identify(struct cpuinfo_x86 *c)
 			cpuid_count(7, 1,
 				    &c->x86_capability[FEATURESET_7a1],
 				    &c->x86_capability[FEATURESET_7b1],
-				    &tmp, &tmp);
+				    &c->x86_capability[FEATURESET_7c1],
+				    &c->x86_capability[FEATURESET_7d1]);
 		if (max_subleaf >= 2)
 			cpuid_count(7, 2,
 				    &tmp, &tmp, &tmp,
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 7915f5826f57..7a896f0e2d92 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -288,6 +288,9 @@ XEN_CPUFEATURE(NSCB,               11*32+ 6) /*A  Null Selector Clears Base (and
 /* Intel-defined CPU features, CPUID level 0x00000007:1.ebx, word 12 */
 XEN_CPUFEATURE(INTEL_PPIN,         12*32+ 0) /*   Protected Processor Inventory Number */
 
+/* Intel-defined CPU features, CPUID level 0x00000007:1.ecx, word 14 */
+/* Intel-defined CPU features, CPUID level 0x00000007:1.edx, word 15 */
+
 /* Intel-defined CPU features, CPUID level 0x00000007:2.edx, word 13 */
 XEN_CPUFEATURE(INTEL_PSFD,         13*32+ 0) /*A  MSR_SPEC_CTRL.PSFD */
 XEN_CPUFEATURE(IPRED_CTRL,         13*32+ 1) /*   MSR_SPEC_CTRL.IPRED_DIS_* */
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index 73a5c330365e..fa98b371eef4 100644
--- a/xen/include/xen/lib/x86/cpuid.h
+++ b/xen/include/xen/lib/x86/cpuid.h
@@ -18,6 +18,8 @@
 #define FEATURESET_e21a  11 /* 0x80000021.eax      */
 #define FEATURESET_7b1   12 /* 0x00000007:1.ebx    */
 #define FEATURESET_7d2   13 /* 0x00000007:2.edx    */
+#define FEATURESET_7c1   14 /* 0x00000007:1.ecx    */
+#define FEATURESET_7d1   15 /* 0x00000007:1.edx    */
 
 struct cpuid_leaf
 {
@@ -194,7 +196,14 @@ struct cpuid_policy
                 uint32_t _7b1;
                 struct { DECL_BITFIELD(7b1); };
             };
-            uint32_t /* c */:32, /* d */:32;
+            union {
+                uint32_t _7c1;
+                struct { DECL_BITFIELD(7c1); };
+            };
+            union {
+                uint32_t _7d1;
+                struct { DECL_BITFIELD(7d1); };
+            };
 
             /* Subleaf 2. */
             uint32_t /* a */:32, /* b */:32, /* c */:32;
@@ -343,6 +352,8 @@ static inline void cpuid_policy_to_featureset(
     fs[FEATURESET_e21a] = p->extd.e21a;
     fs[FEATURESET_7b1] = p->feat._7b1;
     fs[FEATURESET_7d2] = p->feat._7d2;
+    fs[FEATURESET_7c1] = p->feat._7c1;
+    fs[FEATURESET_7d1] = p->feat._7d1;
 }
 
 /* Fill in a CPUID policy from a featureset bitmap. */
@@ -363,6 +374,8 @@ static inline void cpuid_featureset_to_policy(
     p->extd.e21a  = fs[FEATURESET_e21a];
     p->feat._7b1  = fs[FEATURESET_7b1];
     p->feat._7d2  = fs[FEATURESET_7d2];
+    p->feat._7c1  = fs[FEATURESET_7c1];
+    p->feat._7d1  = fs[FEATURESET_7d1];
 }
 
 static inline uint64_t cpuid_policy_xcr0_max(const struct cpuid_policy *p)
-- 
2.11.0

[PATCH 2/2] x86/shskt: Disable CET-SS on parts succeptable to fractured updates

[Andrew Cooper]

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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>

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)
+			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

Re: [PATCH 1/2] x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 1675 bytes --]

On Sat, Dec 31, 2022 at 12:30:06AM +0000, Andrew Cooper wrote:
> We don't actually need ecx yet, but adding it in now will reduce the amount to
> which leaf 7 is out of order in a featureset.
> 
> cpufeatureset.h remains in leaf architectrual order for the sanity of anyone
> trying to locate where to insert new rows.
> 
> 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>
> ---
>  tools/misc/xen-cpuid.c                      | 10 ++++++++++
>  xen/arch/x86/cpu/common.c                   |  3 ++-
>  xen/include/public/arch-x86/cpufeatureset.h |  3 +++
>  xen/include/xen/lib/x86/cpuid.h             | 15 ++++++++++++++-
>  4 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
> index d5833e9ce879..0091a11a67bc 100644
> --- a/tools/misc/xen-cpuid.c
> +++ b/tools/misc/xen-cpuid.c
> @@ -202,6 +202,14 @@ static const char *const str_7b1[32] =
>      [ 0] = "ppin",
>  };
>  
> +static const char *const str_7c1[32] =
> +{
> +};
> +
> +static const char *const str_7d1[32] =
> +{
> +};
> +
>  static const char *const str_7d2[32] =
>  {
>      [ 0] = "intel-psfd",
> @@ -229,6 +237,8 @@ static const struct {
>      { "0x80000021.eax",  "e21a", str_e21a },
>      { "0x00000007:1.ebx", "7b1", str_7b1 },
>      { "0x00000007:2.edx", "7d2", str_7d2 },
> +    { "0x00000007:1.ecx", "7b1", str_7c1 },
> +    { "0x00000007:1.edx", "7b1", str_7d1 },

"7c1" and "7d1" ?



-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] x86/shskt: Disable CET-SS on parts succeptable to fractured updates

[Marek Marczykowski-Górecki]

[-- Attachment #1: Type: text/plain, Size: 8737 bytes --]

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
> 
> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}

[Andrew Cooper]

On 01/01/2023 3:00 pm, Marek Marczykowski-Górecki wrote:
> On Sat, Dec 31, 2022 at 12:30:06AM +0000, Andrew Cooper wrote:
>> diff --git a/tools/misc/xen-cpuid.c b/tools/misc/xen-cpuid.c
>> index d5833e9ce879..0091a11a67bc 100644
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -229,6 +237,8 @@ static const struct {
>>      { "0x80000021.eax",  "e21a", str_e21a },
>>      { "0x00000007:1.ebx", "7b1", str_7b1 },
>>      { "0x00000007:2.edx", "7d2", str_7d2 },
>> +    { "0x00000007:1.ecx", "7b1", str_7c1 },
>> +    { "0x00000007:1.edx", "7b1", str_7d1 },
> "7c1" and "7d1" ?

Fixed, thanks.

~Andrew

Re: [PATCH 2/2] x86/shskt: Disable CET-SS on parts succeptable to fractured updates

[Andrew Cooper]

On 01/01/2023 3:10 pm, Marek Marczykowski-Górecki wrote:
> On Sat, Dec 31, 2022 at 12:30:07AM +0000, Andrew Cooper wrote:
>> 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 ...

Fixed.

>
> Is this file imported from Linux? It uses tabs for indentation, contrary
> to the rest of the Xen code base.

It is a file which originally inherits from Linux, but it probably has
~0% in common any more...

~Andrew

[PATCH 0/2] x86: Work around Shstk fracturing

[Andrew Cooper]

See patch 2 for details.

Andrew Cooper (2):
  x86/cpuid: Infrastructure for leaves 7:1{ecx,edx}
  x86/shskt: Disable CET-SS on parts susceptible to fractured updates

 docs/misc/xen-command-line.pandoc           |  7 ++++-
 tools/libs/light/libxl_cpuid.c              |  2 ++
 tools/misc/xen-cpuid.c                      | 11 +++++++
 xen/arch/x86/cpu/common.c                   | 14 +++++++--
 xen/arch/x86/setup.c                        | 46 ++++++++++++++++++++++++-----
 xen/include/public/arch-x86/cpufeatureset.h |  4 +++
 xen/include/xen/lib/x86/cpuid.h             | 15 +++++++++-
 7 files changed, 86 insertions(+), 13 deletions(-)

-- 
2.11.0