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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxx>

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