| Message ID | 20230629121713.1211-4-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce a REQUIRE_NX Kconfig option | expand |
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
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
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
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
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)