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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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