Re: [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack

[Laszlo Ersek <lersek@redhat.com>]

On 09/11/18 07:16, Jian J Wang wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since PcdSetNxForStack is expired, remove related config code.
> PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD.
> There's no need to add it in code to replace PcdSetNxForStack.
>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  OvmfPkg/PlatformPei/Platform.c      | 1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index 5a78668126..6627d236e0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -340,7 +340,6 @@ NoexecDxeInitialization (
>    )
>  {
>    UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable);
> -  UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack);
>  }
>
>  VOID
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 9c5ad9961c..ef5dcb7693 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -96,7 +96,6 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
>    gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> -  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask
>

I disagree with this change. I explained my reasons in
<https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c6>, but for the
sake of discussion, I'll state the same here:

> Ard's got a point here. Just because one PCD implies another, the
> reverse is not necessarily true.
>
> For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but
> then we had to make it conditional, due to old GRUB variants breaking
> with a non-executable stack. (Please refer to commit d20b06a3afdf,
> "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15).
> Therefore we now default to FALSE, and let the user set it to TRUE on
> the QEMU command line.
>
> In addition, we intentionally inherit a zero
> PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec".
>
> Both of the above configurations satisfy the requirement
>
>   ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &
>     (1 << EfiBootServicesData)) == 0 ||
>    PcdGetBool (PcdSetNxForStack))
>
> i.e. the requirement that "NX for BS data" imply "NX for stack".
>
> If you remove the standalone knob for PcdSetNxForStack, then the
> default behavior of OVMF will continue to work, however the command
> line option, for setting PcdSetNxForStack *only*, will break.

I'd like to add another comment just here (not mentioned in the BZ):

To my understanding, the Heap Guard is a debug feature [1]. Over time,
I've reviewed and regression-tested all the Heap Guard patches that have
crossed my path with the understanding that "this is not enabled by
default in OVMF". With those points in mind, I certainly don't intend to
enable the Heap Guard as a FixedPCD -- which is the only way it can be
enabled -- in the OVMF DSC files anytime soon.

On the other hand, the user should set the stack NX preferably at all
times -- as I wrote above, that was our original idea for the OVMF
default as well, until we were forced to revert it for compatibility's
sake, and to expose the knob to the end-user instead.

With this patch series, the configuration that's currently deemed the
best compromise for OVMF (Heap Guard off, stack NX) would be lost.

[1] http://mid.mail-archive.com/D827630B58408649ACB04F44C510003624DDC8B3@SHSMSX103.ccr.corp.intel.com

Thanks
Laszlo