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

[Laszlo Ersek <lersek@redhat.com>]

On 09/12/18 04:11, Wang, Jian J wrote:
> 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.

Yes, that's what I thought as well. Catch the one case with an assert
and/or CpuDeadLoop() that is an invalid configuration.

> 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.

My understanding has been that it's not about enabling/disabling a CPU
feature, but about marking specific pages as non-executable. Under that
interpretation, it should be possible to mark pages used for stack
purposes as non-executable, and leave everything else executable, even
on x86.

Laszlo

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