[PATCH v3 0/3] Introduce a REQUIRE_NX Kconfig option

[PATCH v3 0/3] Introduce a REQUIRE_NX Kconfig option

[Alejandro Vallejo]

v3:
  * Fixed a Gitlab CI breakage on older toolchains (patch 1)
  * Removed XD_DISABLE override logic from cpu/intel.c
  * Various style fixes to patch 2 (from Andrew's feedback)

This option hardens Xen by forcing it to write secure (NX-enhanced) PTEs
regardless of the runtime NX feature bit in boot_cpu_data. This prevents an
attacker with partial write support from affecting Xen's PTE generation
logic by overriding the NX feature flag. The patch asserts support for the
NX bit in PTEs at boot time and if so short-circuits the cpu_has_nx macro
to 1.

Alejandro Vallejo (3):
  tools: Add __AC() macro to common-macros.h
  x86/boot: Clear XD_DISABLE from the early boot path
  x86: Add Kconfig option to require NX bit support

 tools/include/xen-tools/common-macros.h |  3 ++
 tools/libs/light/libxl_internal.h       |  2 -
 tools/tests/x86_emulator/x86-emulate.h  |  3 --
 xen/arch/x86/Kconfig                    | 16 +++++++
 xen/arch/x86/boot/head.S                | 62 ++++++++++++++++++++++---
 xen/arch/x86/boot/trampoline.S          |  3 +-
 xen/arch/x86/cpu/intel.c                | 16 +++----
 xen/arch/x86/efi/efi-boot.h             |  9 ++++
 xen/arch/x86/include/asm/cpufeature.h   |  3 +-
 xen/arch/x86/include/asm/msr-index.h    |  2 +-
 xen/arch/x86/include/asm/x86-vendors.h  |  6 +--
 11 files changed, 98 insertions(+), 27 deletions(-)

-- 
2.34.1

[PATCH v3 1/3] tools: Add __AC() macro to common-macros.h

[Alejandro Vallejo]

Currently libxl and the x86-emulator tests carry their own versions. Factor
those out into the common macros header so every library can make use of
it. This is required so the following patch can add this macro to a header
used both in Xen and tools/libs.

No functional change.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
 tools/include/xen-tools/common-macros.h | 3 +++
 tools/libs/light/libxl_internal.h       | 2 --
 tools/tests/x86_emulator/x86-emulate.h  | 3 ---
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
index 168691be0e..6d7de1bc0a 100644
--- a/tools/include/xen-tools/common-macros.h
+++ b/tools/include/xen-tools/common-macros.h
@@ -84,4 +84,7 @@
     (type *)((char *)mptr__ - offsetof(type, member));  \
 })
 
+#define __AC(X,Y)   (X##Y)
+#define _AC(X,Y)    __AC(X,Y)
+
 #endif	/* __XEN_TOOLS_COMMON_MACROS__ */
diff --git a/tools/libs/light/libxl_internal.h b/tools/libs/light/libxl_internal.h
index 61f4fe1dec..1cf3d400bf 100644
--- a/tools/libs/light/libxl_internal.h
+++ b/tools/libs/light/libxl_internal.h
@@ -125,8 +125,6 @@
 #define PVSHIM_CMDLINE "pv-shim console=xen,pv"
 
 /* Size macros. */
-#define __AC(X,Y)   (X##Y)
-#define _AC(X,Y)    __AC(X,Y)
 #define MB(_mb)     (_AC(_mb, ULL) << 20)
 #define GB(_gb)     (_AC(_gb, ULL) << 30)
 
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index aa1ed75ec8..350d1a0abf 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -59,9 +59,6 @@
 
 #define cf_check /* No Control Flow Integriy checking */
 
-#define AC_(n,t) (n##t)
-#define _AC(n,t) AC_(n,t)
-
 #ifdef __GCC_ASM_FLAG_OUTPUTS__
 # define ASM_FLAG_OUT(yes, no) yes
 #else
-- 
2.34.1

Re: [PATCH v3 1/3] tools: Add __AC() macro to common-macros.h

[Alejandro Vallejo]

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

On Thu, Jun 29, 2023 at 1:17 PM Alejandro Vallejo <
alejandro.vallejo@cloud.com> wrote:

> Currently libxl and the x86-emulator tests carry their own versions. Factor
> those out into the common macros header so every library can make use of
> it. This is required so the following patch can add this macro to a header
> used both in Xen and tools/libs.
>
> No functional change.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
>  tools/include/xen-tools/common-macros.h | 3 +++
>  tools/libs/light/libxl_internal.h       | 2 --
>  tools/tests/x86_emulator/x86-emulate.h  | 3 ---
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tools/include/xen-tools/common-macros.h
> b/tools/include/xen-tools/common-macros.h
> index 168691be0e..6d7de1bc0a 100644
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -84,4 +84,7 @@
>      (type *)((char *)mptr__ - offsetof(type, member));  \
>  })
>
> +#define __AC(X,Y)   (X##Y)
> +#define _AC(X,Y)    __AC(X,Y)
> +
>  #endif /* __XEN_TOOLS_COMMON_MACROS__ */
> diff --git a/tools/libs/light/libxl_internal.h
> b/tools/libs/light/libxl_internal.h
> index 61f4fe1dec..1cf3d400bf 100644
> --- a/tools/libs/light/libxl_internal.h
> +++ b/tools/libs/light/libxl_internal.h
> @@ -125,8 +125,6 @@
>  #define PVSHIM_CMDLINE "pv-shim console=xen,pv"
>
>  /* Size macros. */
> -#define __AC(X,Y)   (X##Y)
> -#define _AC(X,Y)    __AC(X,Y)
>  #define MB(_mb)     (_AC(_mb, ULL) << 20)
>  #define GB(_gb)     (_AC(_gb, ULL) << 30)
>
> diff --git a/tools/tests/x86_emulator/x86-emulate.h
> b/tools/tests/x86_emulator/x86-emulate.h
> index aa1ed75ec8..350d1a0abf 100644
> --- a/tools/tests/x86_emulator/x86-emulate.h
> +++ b/tools/tests/x86_emulator/x86-emulate.h
> @@ -59,9 +59,6 @@
>
>  #define cf_check /* No Control Flow Integriy checking */
>
> -#define AC_(n,t) (n##t)
> -#define _AC(n,t) AC_(n,t)
> -
>  #ifdef __GCC_ASM_FLAG_OUTPUTS__
>  # define ASM_FLAG_OUT(yes, no) yes
>  #else
> --
> 2.34.1
>

@mantainers

[-- Attachment #2: Type: text/html, Size: 2676 bytes --]

Re: [PATCH v3 1/3] tools: Add __AC() macro to common-macros.h

[Andrew Cooper]

On 29/06/2023 1:17 pm, Alejandro Vallejo wrote:
> Currently libxl and the x86-emulator tests carry their own versions. Factor
> those out into the common macros header so every library can make use of
> it. This is required so the following patch can add this macro to a header
> used both in Xen and tools/libs.
>
> No functional change.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, although ...

> ---
>  tools/include/xen-tools/common-macros.h | 3 +++
>  tools/libs/light/libxl_internal.h       | 2 --
>  tools/tests/x86_emulator/x86-emulate.h  | 3 ---
>  3 files changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/tools/include/xen-tools/common-macros.h b/tools/include/xen-tools/common-macros.h
> index 168691be0e..6d7de1bc0a 100644
> --- a/tools/include/xen-tools/common-macros.h
> +++ b/tools/include/xen-tools/common-macros.h
> @@ -84,4 +84,7 @@
>      (type *)((char *)mptr__ - offsetof(type, member));  \
>  })
>  
> +#define __AC(X,Y)   (X##Y)
> +#define _AC(X,Y)    __AC(X,Y)

... I'll take the opportunity to do style fixes here.

~Andrew

[PATCH v3 2/3] x86/boot: Clear XD_DISABLE from the early boot path

[Alejandro Vallejo]

Intel CPUs have a bit in MSR_IA32_MISC_ENABLE that may prevent the NX bit
from being advertised. Clear it unconditionally if we can't find the NX
feature right away on boot.

The conditions for the MSR being read on early boot are (in this order):

* Long Mode is supported
* NX isn't advertised
* The vendor is Intel

The order of checks has been chosen carefully so a virtualized Xen on a
hypervisor that doesn't emulate that MSR (but supports NX) doesn't triple
fault trying to access the non-existing MSR.

With that done, we can remove the XD_DISABLE checks in the intel-specific
init path (as they are already done in early assembly). Keep a printk to
highlight the fact that NX was forcefully enabled.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
---
v3:
  * In head.S: s/has_nx/got_nx and s/nx_bit/nx
  * Style changes in assembly instructions (spaces + width modifiers)
  * Big comment in head.S replaced
  * Jump directly to .Lno_nx if NX not found and XD_DISABLE not present
  * Restored rdmsrl (previously refactored into rdmsr_safe() in v2) and
    removed XD_DISABLE clearing in C (as it's now done in head.S).
  * Moved printk in intel.c to highlight the XD_DISABLE override even when
    done in head.S
---
 xen/arch/x86/boot/head.S               | 49 ++++++++++++++++++++++----
 xen/arch/x86/cpu/intel.c               | 16 ++++-----
 xen/arch/x86/include/asm/msr-index.h   |  2 +-
 xen/arch/x86/include/asm/x86-vendors.h |  6 ++--
 4 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 9fbd602ea5..0e02c28f37 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -652,16 +652,53 @@ trampoline_setup:
         cpuid
 1:      mov     %edx, CPUINFO_FEATURE_OFFSET(X86_FEATURE_LM) + sym_esi(boot_cpu_data)
 
-        /* Check for NX. Adjust EFER setting if available. */
-        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
-        jnc     1f
-        orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
-1:
-
         /* Check for availability of long mode. */
         bt      $cpufeat_bit(X86_FEATURE_LM),%edx
         jnc     .Lbad_cpu
 
+        /* Check for NX */
+        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
+        jc     .Lgot_nx
+
+        /*
+         * NX appears to be unsupported, but it might be hidden.
+         *
+         * The feature is part of the AMD64 spec, but the very first Intel
+         * 64bit CPUs lacked the feature, and thereafter there was a
+         * firmware knob to disable the feature. Undo the disable if
+         * possible.
+         *
+         * All 64bit Intel CPUs support this MSR. If virtualised, expect
+         * the hypervisor to either emulate the MSR or give us NX.
+         */
+        xor     %eax, %eax
+        cpuid
+        cmp     $X86_VENDOR_INTEL_EBX, %ebx
+        jnz     .Lno_nx
+        cmp     $X86_VENDOR_INTEL_EDX, %edx
+        jnz     .Lno_nx
+        cmp     $X86_VENDOR_INTEL_ECX, %ecx
+        jnz     .Lno_nx
+
+        /* Clear the XD_DISABLE bit */
+        mov    $MSR_IA32_MISC_ENABLE, %ecx
+        rdmsr
+        btr     $2, %edx
+        jnc     .Lno_nx
+        wrmsr
+        orb     $MSR_IA32_MISC_ENABLE_XD_DISABLE >> 32, 4 + sym_esi(trampoline_misc_enable_off)
+
+        /* Check again for NX */
+        mov     $0x80000001, %eax
+        cpuid
+        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
+        jnc     .Lno_nx
+
+.Lgot_nx:
+        /* Adjust EFER given that NX is present */
+        orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
+.Lno_nx:
+
         /* Stash TSC to calculate a good approximation of time-since-boot */
         rdtsc
         mov     %eax,     sym_esi(boot_tsc_stamp)
diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
index 168cd58f36..b2443b6831 100644
--- a/xen/arch/x86/cpu/intel.c
+++ b/xen/arch/x86/cpu/intel.c
@@ -304,24 +304,20 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
 	if (c->x86 == 15 && c->x86_cache_alignment == 64)
 		c->x86_cache_alignment = 128;
 
+	if (bootsym(trampoline_misc_enable_off) &
+	    MSR_IA32_MISC_ENABLE_XD_DISABLE)
+		printk(KERN_INFO
+		       "re-enabled NX (Execute Disable) protection\n");
+
 	/* Unmask CPUID levels and NX if masked: */
 	rdmsrl(MSR_IA32_MISC_ENABLE, misc_enable);
 
-	disable = misc_enable & (MSR_IA32_MISC_ENABLE_LIMIT_CPUID |
-				 MSR_IA32_MISC_ENABLE_XD_DISABLE);
+	disable = misc_enable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID;
 	if (disable) {
 		wrmsrl(MSR_IA32_MISC_ENABLE, misc_enable & ~disable);
 		bootsym(trampoline_misc_enable_off) |= disable;
-		bootsym(trampoline_efer) |= EFER_NXE;
-	}
-
-	if (disable & MSR_IA32_MISC_ENABLE_LIMIT_CPUID)
 		printk(KERN_INFO "revised cpuid level: %d\n",
 		       cpuid_eax(0));
-	if (disable & MSR_IA32_MISC_ENABLE_XD_DISABLE) {
-		write_efer(read_efer() | EFER_NXE);
-		printk(KERN_INFO
-		       "re-enabled NX (Execute Disable) protection\n");
 	}
 
 	/* CPUID workaround for Intel 0F33/0F34 CPU */
diff --git a/xen/arch/x86/include/asm/msr-index.h b/xen/arch/x86/include/asm/msr-index.h
index 2749e433d2..4f861c0bb4 100644
--- a/xen/arch/x86/include/asm/msr-index.h
+++ b/xen/arch/x86/include/asm/msr-index.h
@@ -502,7 +502,7 @@
 #define MSR_IA32_MISC_ENABLE_MONITOR_ENABLE (1<<18)
 #define MSR_IA32_MISC_ENABLE_LIMIT_CPUID  (1<<22)
 #define MSR_IA32_MISC_ENABLE_XTPR_DISABLE (1<<23)
-#define MSR_IA32_MISC_ENABLE_XD_DISABLE	(1ULL << 34)
+#define MSR_IA32_MISC_ENABLE_XD_DISABLE   (_AC(1, ULL) << 34)
 
 #define MSR_IA32_TSC_DEADLINE		0x000006E0
 #define MSR_IA32_ENERGY_PERF_BIAS	0x000001b0
diff --git a/xen/arch/x86/include/asm/x86-vendors.h b/xen/arch/x86/include/asm/x86-vendors.h
index 0a37024cbd..9191da26d7 100644
--- a/xen/arch/x86/include/asm/x86-vendors.h
+++ b/xen/arch/x86/include/asm/x86-vendors.h
@@ -12,9 +12,9 @@
 #define X86_VENDOR_UNKNOWN 0
 
 #define X86_VENDOR_INTEL (1 << 0)
-#define X86_VENDOR_INTEL_EBX 0x756e6547U /* "GenuineIntel" */
-#define X86_VENDOR_INTEL_ECX 0x6c65746eU
-#define X86_VENDOR_INTEL_EDX 0x49656e69U
+#define X86_VENDOR_INTEL_EBX _AC(0x756e6547, U) /* "GenuineIntel" */
+#define X86_VENDOR_INTEL_ECX _AC(0x6c65746e, U)
+#define X86_VENDOR_INTEL_EDX _AC(0x49656e69, U)
 
 #define X86_VENDOR_AMD (1 << 1)
 #define X86_VENDOR_AMD_EBX 0x68747541U /* "AuthenticAMD" */
-- 
2.34.1

Re: [PATCH v3 2/3] x86/boot: Clear XD_DISABLE from the early boot path

[Alejandro Vallejo]

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

On Thu, Jun 29, 2023 at 1:17 PM Alejandro Vallejo <
alejandro.vallejo@cloud.com> wrote:

> Intel CPUs have a bit in MSR_IA32_MISC_ENABLE that may prevent the NX bit
> from being advertised. Clear it unconditionally if we can't find the NX
> feature right away on boot.
>
> The conditions for the MSR being read on early boot are (in this order):
>
> * Long Mode is supported
> * NX isn't advertised
> * The vendor is Intel
>
> The order of checks has been chosen carefully so a virtualized Xen on a
> hypervisor that doesn't emulate that MSR (but supports NX) doesn't triple
> fault trying to access the non-existing MSR.
>
> With that done, we can remove the XD_DISABLE checks in the intel-specific
> init path (as they are already done in early assembly). Keep a printk to
> highlight the fact that NX was forcefully enabled.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> ---
> v3:
>   * In head.S: s/has_nx/got_nx and s/nx_bit/nx
>   * Style changes in assembly instructions (spaces + width modifiers)
>   * Big comment in head.S replaced
>   * Jump directly to .Lno_nx if NX not found and XD_DISABLE not present
>   * Restored rdmsrl (previously refactored into rdmsr_safe() in v2) and
>     removed XD_DISABLE clearing in C (as it's now done in head.S).
>   * Moved printk in intel.c to highlight the XD_DISABLE override even when
>     done in head.S
> ---
>  xen/arch/x86/boot/head.S               | 49 ++++++++++++++++++++++----
>  xen/arch/x86/cpu/intel.c               | 16 ++++-----
>  xen/arch/x86/include/asm/msr-index.h   |  2 +-
>  xen/arch/x86/include/asm/x86-vendors.h |  6 ++--
>  4 files changed, 53 insertions(+), 20 deletions(-)
>

@mantainers

[-- Attachment #2: Type: text/html, Size: 2225 bytes --]

Re: [PATCH v3 2/3] x86/boot: Clear XD_DISABLE from the early boot path

[Andrew Cooper]

On 29/06/2023 1:17 pm, Alejandro Vallejo wrote:
> Intel CPUs have a bit in MSR_IA32_MISC_ENABLE that may prevent the NX bit
> from being advertised. Clear it unconditionally if we can't find the NX
> feature right away on boot.
>
> The conditions for the MSR being read on early boot are (in this order):
>
> * Long Mode is supported
> * NX isn't advertised
> * The vendor is Intel
>
> The order of checks has been chosen carefully so a virtualized Xen on a
> hypervisor that doesn't emulate that MSR (but supports NX) doesn't triple
> fault trying to access the non-existing MSR.
>
> With that done, we can remove the XD_DISABLE checks in the intel-specific
> init path (as they are already done in early assembly). Keep a printk to
> highlight the fact that NX was forcefully enabled.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, with two minor
fixes.

> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 9fbd602ea5..0e02c28f37 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -652,16 +652,53 @@ trampoline_setup:
>          cpuid
>  1:      mov     %edx, CPUINFO_FEATURE_OFFSET(X86_FEATURE_LM) + sym_esi(boot_cpu_data)
>  
> -        /* Check for NX. Adjust EFER setting if available. */
> -        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> -        jnc     1f
> -        orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
> -1:
> -
>          /* Check for availability of long mode. */
>          bt      $cpufeat_bit(X86_FEATURE_LM),%edx
>          jnc     .Lbad_cpu
>  
> +        /* Check for NX */
> +        bt      $cpufeat_bit(X86_FEATURE_NX), %edx
> +        jc     .Lgot_nx
> +
> +        /*
> +         * NX appears to be unsupported, but it might be hidden.
> +         *
> +         * The feature is part of the AMD64 spec, but the very first Intel
> +         * 64bit CPUs lacked the feature, and thereafter there was a
> +         * firmware knob to disable the feature. Undo the disable if
> +         * possible.
> +         *
> +         * All 64bit Intel CPUs support this MSR. If virtualised, expect
> +         * the hypervisor to either emulate the MSR or give us NX.
> +         */
> +        xor     %eax, %eax
> +        cpuid
> +        cmp     $X86_VENDOR_INTEL_EBX, %ebx
> +        jnz     .Lno_nx
> +        cmp     $X86_VENDOR_INTEL_EDX, %edx
> +        jnz     .Lno_nx
> +        cmp     $X86_VENDOR_INTEL_ECX, %ecx
> +        jnz     .Lno_nx
> +
> +        /* Clear the XD_DISABLE bit */
> +        mov    $MSR_IA32_MISC_ENABLE, %ecx

Parameter indention here.

> diff --git a/xen/arch/x86/include/asm/x86-vendors.h b/xen/arch/x86/include/asm/x86-vendors.h
> index 0a37024cbd..9191da26d7 100644
> --- a/xen/arch/x86/include/asm/x86-vendors.h
> +++ b/xen/arch/x86/include/asm/x86-vendors.h
> @@ -12,9 +12,9 @@
>  #define X86_VENDOR_UNKNOWN 0
>  
>  #define X86_VENDOR_INTEL (1 << 0)
> -#define X86_VENDOR_INTEL_EBX 0x756e6547U /* "GenuineIntel" */
> -#define X86_VENDOR_INTEL_ECX 0x6c65746eU
> -#define X86_VENDOR_INTEL_EDX 0x49656e69U
> +#define X86_VENDOR_INTEL_EBX _AC(0x756e6547, U) /* "GenuineIntel" */
> +#define X86_VENDOR_INTEL_ECX _AC(0x6c65746e, U)
> +#define X86_VENDOR_INTEL_EDX _AC(0x49656e69, U)
>  
>  #define X86_VENDOR_AMD (1 << 1)
>  #define X86_VENDOR_AMD_EBX 0x68747541U /* "AuthenticAMD" */

And all vendors should get equivalent _AC() conversions.

Can fix both on commit.

~Andrew

Re: [PATCH v3 2/3] x86/boot: Clear XD_DISABLE from the early boot path

[Andrew Cooper]

On 29/06/2023 1:17 pm, Alejandro Vallejo wrote:
> diff --git a/xen/arch/x86/cpu/intel.c b/xen/arch/x86/cpu/intel.c
> index 168cd58f36..b2443b6831 100644
> --- a/xen/arch/x86/cpu/intel.c
> +++ b/xen/arch/x86/cpu/intel.c
> @@ -304,24 +304,20 @@ static void cf_check early_init_intel(struct cpuinfo_x86 *c)
>  	if (c->x86 == 15 && c->x86_cache_alignment == 64)
>  		c->x86_cache_alignment = 128;
>  
> +	if (bootsym(trampoline_misc_enable_off) &
> +	    MSR_IA32_MISC_ENABLE_XD_DISABLE)
> +		printk(KERN_INFO
> +		       "re-enabled NX (Execute Disable) protection\n");

One other thing, which I'll also fix on commit.

This now prints per CPU on any system where we had to set XD_DISABLE. 
It want's a c == &boot_cpu_data to limit it to once-only.

~Andrew

[PATCH v3 3/3] x86: Add Kconfig option to require NX bit support

[Alejandro Vallejo]

This option hardens Xen by forcing it to write secure (NX-enhanced) PTEs
regardless of the runtime NX feature bit in boot_cpu_data. This prevents an
attacker with partial write support from affecting Xen's PTE generation
logic by overriding the NX feature flag. The patch asserts support for the
NX bit in PTEs at boot time and if so short-circuits the cpu_has_nx macro
to 1.

It has the nice benefit of replacing many instances of runtime checks with
folded constants. This has several knock-on effects that improve codegen,
saving 2.5KiB off the text section.

The config option defaults to OFF for compatibility with previous
behaviour.

Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/Kconfig                  | 16 ++++++++++++++++
 xen/arch/x86/boot/head.S              | 15 ++++++++++++++-
 xen/arch/x86/boot/trampoline.S        |  3 ++-
 xen/arch/x86/efi/efi-boot.h           |  9 +++++++++
 xen/arch/x86/include/asm/cpufeature.h |  3 ++-
 5 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 406445a358..92f3a627da 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -307,6 +307,22 @@ config MEM_SHARING
 	bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED
 	depends on HVM
 
+config REQUIRE_NX
+	bool "Require NX (No eXecute) support"
+	help
+	  No-eXecute (also called XD "eXecute Disable" and DEP "Data
+	  Execution Prevention") is a security feature designed originally
+	  to combat buffer overflow attacks by marking regions of memory
+	  which the CPU must not interpret as instructions.
+
+	  The NX feature exists in every 64bit CPU except for some very
+	  early Pentium 4 Prescott machines.
+
+	  Enabling this option will improve Xen's security by removing
+	  cases where Xen could be tricked into thinking that the feature
+	  was unavailable. However, if enabled, Xen will no longer boot on
+	  any CPU which is lacking NX support.
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 0e02c28f37..2e62d07f43 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -123,6 +123,7 @@ multiboot2_header:
 .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
 .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
 .Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
+.Lno_nx_msg:   .asciz "ERR: Not an NX-capable CPU!"
 
         .section .init.data, "aw", @progbits
         .align 4
@@ -153,6 +154,11 @@ early_error: /* Here to improve the disassembly. */
 .Lnot_aligned:
         add     $sym_offs(.Lbag_alg_msg), %esi
         jmp     .Lget_vtb
+#ifdef CONFIG_REQUIRE_NX
+.Lno_nx:
+        add     $sym_offs(.Lno_nx_msg), %esi
+        jmp     .Lget_vtb
+#endif
 .Lmb2_no_st:
         /*
          * Here we are on EFI platform. vga_text_buffer was zapped earlier
@@ -656,7 +662,12 @@ trampoline_setup:
         bt      $cpufeat_bit(X86_FEATURE_LM),%edx
         jnc     .Lbad_cpu
 
-        /* Check for NX */
+        /*
+         * Check for NX
+         *   - If Xen was compiled requiring it simply assert it's
+         *     supported. The trampoline already has the right constant.
+         *   - Otherwise, update the trampoline EFER mask accordingly.
+         */
         bt      $cpufeat_bit(X86_FEATURE_NX), %edx
         jc     .Lgot_nx
 
@@ -695,9 +706,11 @@ trampoline_setup:
         jnc     .Lno_nx
 
 .Lgot_nx:
+#ifndef CONFIG_REQUIRE_NX
         /* Adjust EFER given that NX is present */
         orb     $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer)
 .Lno_nx:
+#endif
 
         /* Stash TSC to calculate a good approximation of time-since-boot */
         rdtsc
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index c6005fa33d..b8ab0ffdcb 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -147,7 +147,8 @@ GLOBAL(trampoline_misc_enable_off)
 
 /* EFER OR-mask for boot paths.  SCE conditional on PV support, NX added when available. */
 GLOBAL(trampoline_efer)
-        .long   EFER_LME | (EFER_SCE * IS_ENABLED(CONFIG_PV))
+        .long   EFER_LME | (EFER_SCE * IS_ENABLED(CONFIG_PV)) | \
+                (EFER_NXE * IS_ENABLED(CONFIG_REQUIRE_NX))
 
 GLOBAL(trampoline_xen_phys_start)
         .long   0
diff --git a/xen/arch/x86/efi/efi-boot.h b/xen/arch/x86/efi/efi-boot.h
index c94e53d139..84700559bb 100644
--- a/xen/arch/x86/efi/efi-boot.h
+++ b/xen/arch/x86/efi/efi-boot.h
@@ -751,6 +751,15 @@ static void __init efi_arch_cpu(void)
     {
         caps[FEATURESET_e1d] = cpuid_edx(0x80000001);
 
+        /*
+         * This check purposefully doesn't use cpu_has_nx because
+         * cpu_has_nx bypasses the boot_cpu_data read if Xen was compiled
+         * with CONFIG_REQUIRE_NX
+         */
+        if ( IS_ENABLED(CONFIG_REQUIRE_NX) &&
+             !boot_cpu_has(X86_FEATURE_NX) )
+            blexit(L"This Xen build requires NX bit support.");
+
         if ( cpu_has_nx )
             trampoline_efer |= EFER_NXE;
     }
diff --git a/xen/arch/x86/include/asm/cpufeature.h b/xen/arch/x86/include/asm/cpufeature.h
index e2cb8f3cc7..64e1dad225 100644
--- a/xen/arch/x86/include/asm/cpufeature.h
+++ b/xen/arch/x86/include/asm/cpufeature.h
@@ -91,7 +91,8 @@ static inline bool boot_cpu_has(unsigned int feat)
 #define cpu_has_hypervisor      boot_cpu_has(X86_FEATURE_HYPERVISOR)
 
 /* CPUID level 0x80000001.edx */
-#define cpu_has_nx              boot_cpu_has(X86_FEATURE_NX)
+#define cpu_has_nx              (IS_ENABLED(CONFIG_REQUIRE_NX) || \
+                                 boot_cpu_has(X86_FEATURE_NX))
 #define cpu_has_page1gb         boot_cpu_has(X86_FEATURE_PAGE1GB)
 #define cpu_has_rdtscp          boot_cpu_has(X86_FEATURE_RDTSCP)
 #define cpu_has_3dnow_ext       boot_cpu_has(X86_FEATURE_3DNOWEXT)
-- 
2.34.1

Re: [PATCH v3 3/3] x86: Add Kconfig option to require NX bit support

[Alejandro Vallejo]

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

On Thu, Jun 29, 2023 at 1:17 PM Alejandro Vallejo <
alejandro.vallejo@cloud.com> wrote:

> This option hardens Xen by forcing it to write secure (NX-enhanced) PTEs
> regardless of the runtime NX feature bit in boot_cpu_data. This prevents an
> attacker with partial write support from affecting Xen's PTE generation
> logic by overriding the NX feature flag. The patch asserts support for the
> NX bit in PTEs at boot time and if so short-circuits the cpu_has_nx macro
> to 1.
>
> It has the nice benefit of replacing many instances of runtime checks with
> folded constants. This has several knock-on effects that improve codegen,
> saving 2.5KiB off the text section.
>
> The config option defaults to OFF for compatibility with previous
> behaviour.
>
> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/Kconfig                  | 16 ++++++++++++++++
>  xen/arch/x86/boot/head.S              | 15 ++++++++++++++-
>  xen/arch/x86/boot/trampoline.S        |  3 ++-
>  xen/arch/x86/efi/efi-boot.h           |  9 +++++++++
>  xen/arch/x86/include/asm/cpufeature.h |  3 ++-
>  5 files changed, 43 insertions(+), 3 deletions(-)
>

@mantainers

[-- Attachment #2: Type: text/html, Size: 1813 bytes --]

Re: [PATCH v3 3/3] x86: Add Kconfig option to require NX bit support

[Jan Beulich]

On 29.06.2023 14:17, Alejandro Vallejo wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -123,6 +123,7 @@ multiboot2_header:
>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>  .Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
> +.Lno_nx_msg:   .asciz "ERR: Not an NX-capable CPU!"
>  
>          .section .init.data, "aw", @progbits
>          .align 4
> @@ -153,6 +154,11 @@ early_error: /* Here to improve the disassembly. */
>  .Lnot_aligned:
>          add     $sym_offs(.Lbag_alg_msg), %esi
>          jmp     .Lget_vtb
> +#ifdef CONFIG_REQUIRE_NX
> +.Lno_nx:
> +        add     $sym_offs(.Lno_nx_msg), %esi
> +        jmp     .Lget_vtb
> +#endif

Since I'm in the process of introducing more such paths (for the x86-64-v<N>
series), I'm curious: Have you actually had success with getting any output
from this code path? I see unreadable output come through serial (provided
it's the normal com1 I/O port location where the serial port is), which
likely is because baud rate wasn't configured yet, and hence I might have
success by changing the config of the receiving side. And I see nothing at
all on the screen. While kind of expected when in graphics mode, I wonder
whether this ever worked, or whether this has simply bitrotted because of
never actually coming into play.

Jan

Re: [PATCH v3 3/3] x86: Add Kconfig option to require NX bit support

[Jan Beulich]

On 18.07.2023 15:19, Jan Beulich wrote:
> On 29.06.2023 14:17, Alejandro Vallejo wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -123,6 +123,7 @@ multiboot2_header:
>>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
>>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
>>  .Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
>> +.Lno_nx_msg:   .asciz "ERR: Not an NX-capable CPU!"
>>  
>>          .section .init.data, "aw", @progbits
>>          .align 4
>> @@ -153,6 +154,11 @@ early_error: /* Here to improve the disassembly. */
>>  .Lnot_aligned:
>>          add     $sym_offs(.Lbag_alg_msg), %esi
>>          jmp     .Lget_vtb
>> +#ifdef CONFIG_REQUIRE_NX
>> +.Lno_nx:
>> +        add     $sym_offs(.Lno_nx_msg), %esi
>> +        jmp     .Lget_vtb
>> +#endif
> 
> Since I'm in the process of introducing more such paths (for the x86-64-v<N>
> series), I'm curious: Have you actually had success with getting any output
> from this code path? I see unreadable output come through serial (provided
> it's the normal com1 I/O port location where the serial port is), which
> likely is because baud rate wasn't configured yet, and hence I might have
> success by changing the config of the receiving side. And I see nothing at
> all on the screen. While kind of expected when in graphics mode, I wonder
> whether this ever worked, or whether this has simply bitrotted because of
> never actually coming into play.

Pretty clearly this was broken in the course of adding MB2 support, by
b28044226e1c using %esi as the "relocation base" after already having
clobbered it. I'm working on a fix.

Jan

Re: [PATCH v3 3/3] x86: Add Kconfig option to require NX bit support

[Alejandro Vallejo]

On Wed, Jul 19, 2023 at 08:13:27AM +0200, Jan Beulich wrote:
> On 18.07.2023 15:19, Jan Beulich wrote:
> > On 29.06.2023 14:17, Alejandro Vallejo wrote:
> >> --- a/xen/arch/x86/boot/head.S
> >> +++ b/xen/arch/x86/boot/head.S
> >> @@ -123,6 +123,7 @@ multiboot2_header:
> >>  .Lbad_ldr_nih: .asciz "ERR: EFI ImageHandle is not provided by bootloader!"
> >>  .Lbad_efi_msg: .asciz "ERR: EFI IA-32 platforms are not supported!"
> >>  .Lbag_alg_msg: .asciz "ERR: Xen must be loaded at a 2Mb boundary!"
> >> +.Lno_nx_msg:   .asciz "ERR: Not an NX-capable CPU!"
> >>  
> >>          .section .init.data, "aw", @progbits
> >>          .align 4
> >> @@ -153,6 +154,11 @@ early_error: /* Here to improve the disassembly. */
> >>  .Lnot_aligned:
> >>          add     $sym_offs(.Lbag_alg_msg), %esi
> >>          jmp     .Lget_vtb
> >> +#ifdef CONFIG_REQUIRE_NX
> >> +.Lno_nx:
> >> +        add     $sym_offs(.Lno_nx_msg), %esi
> >> +        jmp     .Lget_vtb
> >> +#endif
> > 
> > Since I'm in the process of introducing more such paths (for the x86-64-v<N>
> > series), I'm curious: Have you actually had success with getting any output
> > from this code path? I see unreadable output come through serial (provided
> > it's the normal com1 I/O port location where the serial port is), which
> > likely is because baud rate wasn't configured yet, and hence I might have
> > success by changing the config of the receiving side. And I see nothing at
> > all on the screen. While kind of expected when in graphics mode, I wonder
> > whether this ever worked, or whether this has simply bitrotted because of
> > never actually coming into play.
I hacked the code to exercise the XD_DISABLE code path, but didn't try to
exercise the failure paths, I'm afraid. Sorry.
> 
> Pretty clearly this was broken in the course of adding MB2 support, by
> b28044226e1c using %esi as the "relocation base" after already having
> clobbered it. I'm working on a fix.
> 
> Jan
Uh-oh. Good catch.

Alejandro

Re: [PATCH v3 0/3] Introduce a REQUIRE_NX Kconfig option

[Alejandro Vallejo]

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

On Thu, Jun 29, 2023 at 1:17 PM Alejandro Vallejo <
alejandro.vallejo@cloud.com> wrote:

> v3:
>   * Fixed a Gitlab CI breakage on older toolchains (patch 1)
>   * Removed XD_DISABLE override logic from cpu/intel.c
>   * Various style fixes to patch 2 (from Andrew's feedback)
>
> This option hardens Xen by forcing it to write secure (NX-enhanced) PTEs
> regardless of the runtime NX feature bit in boot_cpu_data. This prevents an
> attacker with partial write support from affecting Xen's PTE generation
> logic by overriding the NX feature flag. The patch asserts support for the
> NX bit in PTEs at boot time and if so short-circuits the cpu_has_nx macro
> to 1.
>
> Alejandro Vallejo (3):
>   tools: Add __AC() macro to common-macros.h
>   x86/boot: Clear XD_DISABLE from the early boot path
>   x86: Add Kconfig option to require NX bit support
>
>  tools/include/xen-tools/common-macros.h |  3 ++
>  tools/libs/light/libxl_internal.h       |  2 -
>  tools/tests/x86_emulator/x86-emulate.h  |  3 --
>  xen/arch/x86/Kconfig                    | 16 +++++++
>  xen/arch/x86/boot/head.S                | 62 ++++++++++++++++++++++---
>  xen/arch/x86/boot/trampoline.S          |  3 +-
>  xen/arch/x86/cpu/intel.c                | 16 +++----
>  xen/arch/x86/efi/efi-boot.h             |  9 ++++
>  xen/arch/x86/include/asm/cpufeature.h   |  3 +-
>  xen/arch/x86/include/asm/msr-index.h    |  2 +-
>  xen/arch/x86/include/asm/x86-vendors.h  |  6 +--
>  11 files changed, 98 insertions(+), 27 deletions(-)
>
> --
> 2.34.1
>
> Adding CCs here because I forgot to run the add_maintainers.pl script
before sending. Ugh...

Alejandro

[-- Attachment #2: Type: text/html, Size: 2150 bytes --]