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

["Wang, Jian J" <jian.j.wang@intel.com>]

Hi Laszlo and Ard,

Retiring the PcdSetNxForStack is not the intention of this patch series. It's just
a side effect of solving problem stated in BZ#1116. Sorry again for the misleading
title. I'm not insisting that we have to remove this PCD anyway (My personal
opinion. Others might have different ones). 

I think I understand the importance of PcdSetNxForStack to arm/aarch64 now. And I
agree that it'd be better to enable NX for stack independent of enabling NX for
EfiBootServcesData memory. But since the stack is type of EfiBootServicesData,
I think we should avoid any confusion in enabling/disabling NX for them. That's what
BZ#1116 tries to complain about. But I'm still not clear any concrete suggestion on
how to solve the BZ#1116 from all comment so far, if my patch series cannot satisfy
all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think. (It doesn't
imply we must remove it.)

As I commented in the BZ#1116, maybe we can just simply assert if there's one
trying to disable NX for stack but enable NX for EfiBootServicesData at the same time,
because it doesn't make sense. I think all other setting combinations won't have
such confusion and don't need to take care specifically.

And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs have bits
set. Current DxeIpl will just enable NX for PcdSetNxForStack only.

Regards,
Jian

From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Tuesday, September 11, 2018 11:53 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack

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