| Message ID | 20230601174327.11401-1-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86: Add Kconfig option to require NX bit support | expand |
On 01.06.2023 19:43, Alejandro Vallejo wrote: > This allows replacing many instances of runtime checks with folded > constants. The patch asserts support for the NX bit in PTEs at boot time > and if so short-circuits cpu_has_nx to 1. This has several knock-on effects > that improve codegen: > * _PAGE_NX matches _PAGE_NX_BIT, optimising the macro to a constant. > * Many PAGE_HYPERVISOR_X are also folded into constants > * A few if ( cpu_has_nx ) statements are optimised out > > We save 2.5KiB off the text section and remove the runtime dependency for > applying NX, which hardens our security posture. The config option defaults > to OFF for compatibility with previous behaviour. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> At a guess this may want a Suggested-by: Andrew? > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -307,6 +307,16 @@ config MEM_SHARING > bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED > depends on HVM > > +config REQUIRE_NX_BIT I don't think "_BIT" in the name is necessary or useful. > + def_bool n > + prompt "Require NX bit support" Just bool "Require NX bit support" please. > + ---help--- In new code just "help" please. > @@ -151,6 +152,11 @@ not_multiboot: > .Lnot_aligned: > add $sym_offs(.Lbag_alg_msg),%esi # Error message > jmp .Lget_vtb > +#if IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > +no_nx_bit: .Lno_nx_bit (no need for this to end up in the symbol table, just like most other labels around here). > @@ -647,11 +653,18 @@ 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. */ > + /* > + * 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 > - jnc 1f > + jnc no_nx_bit > +#if !IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > orb $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer) > -1: > +no_nx_bit: > +#endif > > /* Check for availability of long mode. */ > bt $cpufeat_bit(X86_FEATURE_LM),%edx I think it would be nice if this check came ahead of the NX one, as LM availability is quite a bit more important (and hence imo lack thereof wants diagnosing first). Especially as the re-ordering looks to be entirely trivial. > --- 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_BIT > + */ > + if ( IS_ENABLED(CONFIG_REQUIRE_NX_BIT) && > + !boot_cpu_has(X86_FEATURE_NX) ) > + blexit(L"This Xen build requires NX bit support"); Nit: Nearby uses of blexit() suggest there wants to be a full stop. Jan
On 02/06/2023 9:31 am, Jan Beulich wrote: > On 01.06.2023 19:43, Alejandro Vallejo wrote: >> This allows replacing many instances of runtime checks with folded >> constants. The patch asserts support for the NX bit in PTEs at boot time >> and if so short-circuits cpu_has_nx to 1. This has several knock-on effects >> that improve codegen: >> * _PAGE_NX matches _PAGE_NX_BIT, optimising the macro to a constant. >> * Many PAGE_HYPERVISOR_X are also folded into constants >> * A few if ( cpu_has_nx ) statements are optimised out >> >> We save 2.5KiB off the text section and remove the runtime dependency for >> applying NX, which hardens our security posture. The config option defaults >> to OFF for compatibility with previous behaviour. >> >> Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > At a guess this may want a Suggested-by: Andrew? Well - it was a work item off the backlog, and a one-liner at that. I wouldn't have said an explicit tag was warranted simply because I put the backlog together. ~Andrew
Hi, Sure to everything. A couple of notes: On Fri, Jun 02, 2023 at 10:31:08AM +0200, Jan Beulich wrote: > > + def_bool n > > + prompt "Require NX bit support" > > Just > > bool "Require NX bit support" > > please. I didn't realize Kconfig defaulted to 'n'. That's neat, thanks. > > > @@ -151,6 +152,11 @@ not_multiboot: > > .Lnot_aligned: > > add $sym_offs(.Lbag_alg_msg),%esi # Error message > > jmp .Lget_vtb > > +#if IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > > +no_nx_bit: > > .Lno_nx_bit (no need for this to end up in the symbol table, just like > most other labels around here). There's a bunch of others in that general area with global symbols. I'll modify bad_cpu -> .Lbad_cpu as well while dealing with the next suggestion about reordering the NX and Long Mode checks. Alejandro
On 01/06/2023 6:43 pm, Alejandro Vallejo wrote: > This allows replacing many instances of runtime checks with folded > constants. The patch asserts support for the NX bit in PTEs at boot time > and if so short-circuits cpu_has_nx to 1. This has several knock-on effects > that improve codegen: > * _PAGE_NX matches _PAGE_NX_BIT, optimising the macro to a constant. > * Many PAGE_HYPERVISOR_X are also folded into constants > * A few if ( cpu_has_nx ) statements are optimised out > > We save 2.5KiB off the text section and remove the runtime dependency for > applying NX, which hardens our security posture. The config option defaults > to OFF for compatibility with previous behaviour. While this is all true, I'd say it's not emphasising the correct point. Right now, any attacker with a partial write primitive who can clear the NX feature bit in boot_cpu_info will cause Xen to unintentionally write insecure PTEs. (Or for that matter, a memory corruption bug in Xen.) NX exists on all 64bit CPUs other than early Pentium 4's, and people who don't care about those CPUs can meaningfully improve their security defence-in-depth by enabling this option. The fact we also save 2.5k of code is a nice bonus, but not relevant. People aren't going to turn this option on to save code - they're going to turn it on for the extra security. It's fine to mention, but the code gen improvements should be one sentence max, not the majority of the commit message. > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > xen/arch/x86/Kconfig | 10 ++++++++++ > xen/arch/x86/boot/head.S | 19 ++++++++++++++++--- > 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, 39 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig > index 406445a358..0983915372 100644 > --- a/xen/arch/x86/Kconfig > +++ b/xen/arch/x86/Kconfig > @@ -307,6 +307,16 @@ config MEM_SHARING > bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED > depends on HVM > > +config REQUIRE_NX_BIT > + def_bool n > + prompt "Require NX bit support" > + ---help--- > + Makes Xen require NX bit support on page table entries. This > + allows the resulting code to have folded constants where > + otherwise branches are required, yielding a smaller binary as a > + result. Requiring NX trades compatibility with older CPUs for > + improvements in speed and code size. The intended audience here is different. x86 developers will know what this means from the name alone, and won't read the help. It's distro packagers and end users who need to be able to read this and decide whether to turn it on or not. Therefore, it needs to read something more like this: 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 almost 64bit capable CPUs, except XXX [TBC - we might be extremely lucky here. Questions pending with people]. 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 09bebf8635..8414266281 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_bit_msg: .asciz "ERR: Not an NX-bit capable CPU!" > > .section .init.data, "aw", @progbits > .align 4 > @@ -151,6 +152,11 @@ not_multiboot: > .Lnot_aligned: > add $sym_offs(.Lbag_alg_msg),%esi # Error message > jmp .Lget_vtb > +#if IS_ENABLED(CONFIG_REQUIRE_NX_BIT) This doesn't need to be IS_ENABLED(). #if will DTRT for a non-existent symbol by considering such to be 0. IS_ENABLED() is only required for cases where you need an explicit 0 or 1 at the end, which is typically only in real C code, and for initialising of constants. > +no_nx_bit: > + add $sym_offs(.Lno_nx_bit_msg),%esi # Error message The "# Error message" is useless as a comment. Don't propagate it from the other bad examples. (I already had some cleanup planned here from Roger's patch adding not_aligned.) ~Andrew
On 02.06.2023 14:05, Andrew Cooper wrote: > On 01/06/2023 6:43 pm, Alejandro Vallejo wrote: >> @@ -151,6 +152,11 @@ not_multiboot: >> .Lnot_aligned: >> add $sym_offs(.Lbag_alg_msg),%esi # Error message >> jmp .Lget_vtb >> +#if IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > > This doesn't need to be IS_ENABLED(). #if will DTRT for a non-existent > symbol by considering such to be 0. And then it really should be #ifdef, not #if (like for all "real" Kconfig symbols). Jan
On 01/06/2023 6:43 pm, Alejandro Vallejo wrote: > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > index 09bebf8635..8414266281 100644 > --- a/xen/arch/x86/boot/head.S > +++ b/xen/arch/x86/boot/head.S > @@ -647,11 +653,18 @@ 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. */ > + /* > + * 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 > - jnc 1f > + jnc no_nx_bit > +#if !IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > orb $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer) > -1: > +no_nx_bit: > +#endif It occurs to me... This will prevent Xen booting in firmware configurations where XD-Disable is active, despite Xen having unconditional logic to turn XD off later in boot. Linux deals with this in verify_cpu() (early asm) along with a FMS check protecting the access to MSR_MISC_ENABLE, rather than using rdmsr_safe() and catching the #GP. In terms of which CPUs are a problem, we almost got very lucky. NX is part of the AMD64 spec, and all AMD, VIA, Centaur and Intel Atoms have this property. 64bit and XD were both added midway through the Pentium 4 era, and appear in the Prescott E0 stepping. However, it appears that the prior stepping, D0, had 64bit but was only unlocked for certain OEMs. (At the time, Intel were still trying to push Itaniaum as the future, and were trying hard not to go the x86_64 route.) Specifically, https://en.wikipedia.org/wiki/List_of_Intel_Xeon_processors_(NetBurst-based)#%22Nocona%22_(90_nm) is the suspected problem set. So, I think this does want to turn into a series, with the first patch moving the XD-disable logic into this path, after which I think there is a strong case to be made for defaulting CONFIG_REQUIRE_NX to yes. ~Andrew
On Fri, Jun 02, 2023 at 03:22:20PM +0100, Andrew Cooper wrote: > On 01/06/2023 6:43 pm, Alejandro Vallejo wrote: > > diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S > > index 09bebf8635..8414266281 100644 > > --- a/xen/arch/x86/boot/head.S > > +++ b/xen/arch/x86/boot/head.S > > @@ -647,11 +653,18 @@ 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. */ > > + /* > > + * 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 > > - jnc 1f > > + jnc no_nx_bit > > +#if !IS_ENABLED(CONFIG_REQUIRE_NX_BIT) > > orb $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer) > > -1: > > +no_nx_bit: > > +#endif > > It occurs to me... This will prevent Xen booting in firmware > configurations where XD-Disable is active, despite Xen having > unconditional logic to turn XD off later in boot. In practice setting/clearing that bit was done through a BIOS configuration knob AFAIR, so I wouldn't be too worried about forcing it open. > > Linux deals with this in verify_cpu() (early asm) along with a FMS check > protecting the access to MSR_MISC_ENABLE, rather than using rdmsr_safe() > and catching the #GP. > On a related note, we don't use rdmsr_safe() either. We just hope it exists on any Intel CPU. It fortunately does on any Intel CPU we care about because it was introduced shortly before Pentium 4 (Netburst), so we're fine since we mandate long mode. > > In terms of which CPUs are a problem, we almost got very lucky. NX is > part of the AMD64 spec, and all AMD, VIA, Centaur and Intel Atoms have > this property. 64bit and XD were both added midway through the Pentium > 4 era, and appear in the Prescott E0 stepping. > > However, it appears that the prior stepping, D0, had 64bit but was only > unlocked for certain OEMs. (At the time, Intel were still trying to > push Itaniaum as the future, and were trying hard not to go the x86_64 > route.) > > Specifically, > https://en.wikipedia.org/wiki/List_of_Intel_Xeon_processors_(NetBurst-based)#%22Nocona%22_(90_nm) > is the suspected problem set. > > > So, I think this does want to turn into a series, with the first patch > moving the XD-disable logic into this path, I agree. Will do. > after which I think there is > a strong case to be made for defaulting CONFIG_REQUIRE_NX to yes. > > ~Andrew A machine with long mode and no NX would be exceedingly rare, that's for sure. Alejandro
On 02/06/2023 5:08 pm, Alejandro Vallejo wrote: > On Fri, Jun 02, 2023 at 03:22:20PM +0100, Andrew Cooper wrote: >> Linux deals with this in verify_cpu() (early asm) along with a FMS check >> protecting the access to MSR_MISC_ENABLE, rather than using rdmsr_safe() >> and catching the #GP. > On a related note, we don't use rdmsr_safe() either. We just hope it exists > on any Intel CPU. It fortunately does on any Intel CPU we care about > because it was introduced shortly before Pentium 4 (Netburst), so we're > fine since we mandate long mode. Oh, good point. Yeah, that's fine, but only try reading it in the case that we've found LM, not NX, and GenuineIntel. There are old versions of Xen which don't emulate the MSR at all, and the only reason Xen does emulate it in all guests is for a CPUID-faulting corner case. The same assumptions are unlikely to hold for other virtualised cases. Failing with a clear "NX not available" is strictly preferable to triple faulting. ~Andrew
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig index 406445a358..0983915372 100644 --- a/xen/arch/x86/Kconfig +++ b/xen/arch/x86/Kconfig @@ -307,6 +307,16 @@ config MEM_SHARING bool "Xen memory sharing support (UNSUPPORTED)" if UNSUPPORTED depends on HVM +config REQUIRE_NX_BIT + def_bool n + prompt "Require NX bit support" + ---help--- + Makes Xen require NX bit support on page table entries. This + allows the resulting code to have folded constants where + otherwise branches are required, yielding a smaller binary as a + result. Requiring NX trades compatibility with older CPUs for + improvements in speed and code size. + endmenu source "common/Kconfig" diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S index 09bebf8635..8414266281 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_bit_msg: .asciz "ERR: Not an NX-bit capable CPU!" .section .init.data, "aw", @progbits .align 4 @@ -151,6 +152,11 @@ not_multiboot: .Lnot_aligned: add $sym_offs(.Lbag_alg_msg),%esi # Error message jmp .Lget_vtb +#if IS_ENABLED(CONFIG_REQUIRE_NX_BIT) +no_nx_bit: + add $sym_offs(.Lno_nx_bit_msg),%esi # Error message + jmp .Lget_vtb +#endif .Lmb2_no_st: /* * Here we are on EFI platform. vga_text_buffer was zapped earlier @@ -647,11 +653,18 @@ 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. */ + /* + * 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 - jnc 1f + jnc no_nx_bit +#if !IS_ENABLED(CONFIG_REQUIRE_NX_BIT) orb $EFER_NXE >> 8, 1 + sym_esi(trampoline_efer) -1: +no_nx_bit: +#endif /* Check for availability of long mode. */ bt $cpufeat_bit(X86_FEATURE_LM),%edx diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S index c6005fa33d..b964031085 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_BIT)) 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..641d6996c9 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_BIT + */ + if ( IS_ENABLED(CONFIG_REQUIRE_NX_BIT) && + !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 ace31e3b1f..166f480bbc 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_BIT) || \ + 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)
This allows replacing many instances of runtime checks with folded constants. The patch asserts support for the NX bit in PTEs at boot time and if so short-circuits cpu_has_nx to 1. This has several knock-on effects that improve codegen: * _PAGE_NX matches _PAGE_NX_BIT, optimising the macro to a constant. * Many PAGE_HYPERVISOR_X are also folded into constants * A few if ( cpu_has_nx ) statements are optimised out We save 2.5KiB off the text section and remove the runtime dependency for applying NX, which hardens our security posture. The config option defaults to OFF for compatibility with previous behaviour. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- xen/arch/x86/Kconfig | 10 ++++++++++ xen/arch/x86/boot/head.S | 19 ++++++++++++++++--- 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, 39 insertions(+), 5 deletions(-)