[PATCH 0/5] expire the use of PcdSetNxForStack

[PATCH 0/5] expire the use of PcdSetNxForStack

[Jian J Wang]

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116

Since the stack memory is allocated as EfiBootServicesData, its NX protection
can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
table entries mapping the stack memory.

Jian J Wang (5):
  MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
  OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
  OvmfPkg: expire the use of PcdSetNxForStack
  ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
  MdeModulePkg: expire PcdSetNxForStack

 ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
 MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
 MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
 OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
 OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
 OvmfPkg/OvmfPkgX64.dsc                           |  1 -
 OvmfPkg/PlatformPei/Platform.c                   |  1 -
 OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
 13 files changed, 22 insertions(+), 35 deletions(-)

-- 
2.16.2.windows.1

[PATCH 1/5] MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack

[Jian J Wang]

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116

Since the stack memory is allocated as EfiBootServicesData, its NX protection
can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
table entries mapping the stack memory.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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>
---
 MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
 MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
 MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
 MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
 MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
 5 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
index 176d361f19..d44b845b76 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c
@@ -45,7 +45,11 @@ HandOffToDxeCore (
   BaseOfStack = AllocatePages (EFI_SIZE_TO_PAGES (STACK_SIZE));
   ASSERT (BaseOfStack != NULL);
 
-  if (PcdGetBool (PcdSetNxForStack)) {
+  //
+  // Set stack to non-executable, if EfiBootServicesData type of memory is
+  // set for NX protection.
+  //
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT4) != 0) {
     Status = ArmSetMemoryRegionNoExec ((UINTN)BaseOfStack, STACK_SIZE);
     ASSERT_EFI_ERROR (Status);
   }
diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
index fd82657404..44b6ea84ff 100644
--- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
+++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf
@@ -116,7 +116,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                       ## CONSUMES
 
 [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## SOMETIMES_CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy         ## SOMETIMES_CONSUMES
 
 [Depex]
   gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid
diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
index d28baa3615..854078e6dd 100644
--- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c
@@ -245,7 +245,8 @@ ToBuildPageTable (
     return TRUE;
   }
 
-  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable ()) {
+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 &&
+      IsExecuteDisableBitAvailable ()) {
     return TRUE;
   }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
index 81efcfe93d..eb53bc9417 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c
@@ -94,7 +94,7 @@ HandOffToDxeCore (
     // Set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE
     // for the DxeIpl and the DxeCore are both X64.
     //
-    ASSERT (PcdGetBool (PcdSetNxForStack) == FALSE);
+    ASSERT (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) == 0);
     ASSERT (PcdGetBool (PcdCpuStackGuard) == FALSE);
   }
 
diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
index 496e219913..27e9d6955d 100644
--- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
+++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c
@@ -152,7 +152,11 @@ ToSplitPageTable (
     }
   }
 
-  if (PcdGetBool (PcdSetNxForStack)) {
+  //
+  // Set stack to non-executable, if EfiBootServicesData type of memory is
+  // set for NX protection.
+  //
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT4) != 0) {
     if ((Address < StackBase + StackSize) && ((Address + Size) > StackBase)) {
       return TRUE;
     }
@@ -314,7 +318,11 @@ Split2MPageTo4K (
       PageTableEntry->Bits.Present = 1;
     }
 
-    if (PcdGetBool (PcdSetNxForStack)
+    //
+    // Set stack to non-executable, if EfiBootServicesData type of memory is
+    // set for NX protection.
+    //
+    if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT4) != 0
         && (PhysicalAddress4K >= StackBase)
         && (PhysicalAddress4K < StackBase + StackSize)) {
       //
@@ -755,7 +763,7 @@ CreateIdentityMappingPageTables (
   //
   EnablePageTableProtection ((UINTN)PageMap, TRUE);
 
-  if (PcdGetBool (PcdSetNxForStack)) {
+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
     EnableExecuteDisableBit ();
   }
 
-- 
2.16.2.windows.1

Re: [PATCH 1/5] MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack

[Ni, Ruiyu]

On 9/11/2018 1:16 PM, Jian J Wang wrote:
> +    if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & BIT4) != 0

I suggest to use (1 << EfiBootServicesData) to replace BIT4.



-- 
Thanks,
Ray

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

[Jian J Wang]

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
-- 
2.16.2.windows.1

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

[Laszlo Ersek]

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

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

[Wang, Jian J]

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

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

[Laszlo Ersek]

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
> 

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

[Wang, Jian J]

Laszlo,

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

I mentioned that because, for x86 cpu, IA32_EFER.NXE must be set before
marking any page as non-executable. But it's not true for current implementation
relating to PcdDxeNxMemoryProtectionPolicy. Anyway, it's not the key
point of what we've been talking about.

Thank you very much for the comments. I'll send out new patch soon.

Regards,
Jian

From: Laszlo Ersek [mailto:lersek@redhat.com] 
Sent: Wednesday, September 12, 2018 6:42 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/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
> 

[PATCH 3/5] OvmfPkg: expire the use of PcdSetNxForStack

[Jian J Wang]

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116

Since PcdSetNxForStack is expired, remove related references in dsc file.

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/OvmfPkgIa32.dsc    | 1 -
 OvmfPkg/OvmfPkgIa32X64.dsc | 1 -
 OvmfPkg/OvmfPkgX64.dsc     | 1 -
 3 files changed, 3 deletions(-)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 9f07e75050..1eaefbfd6e 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -559,7 +559,6 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
   # Noexec settings for DXE.
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index a4eaeb808c..6f0424c862 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -567,7 +567,6 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
   # Noexec settings for DXE.
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index aa3efc5e73..da6b1293e3 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -566,7 +566,6 @@
   gUefiOvmfPkgTokenSpaceGuid.PcdQemuSmbiosValidated|FALSE
 
   # Noexec settings for DXE.
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable|FALSE
 
   # UefiCpuPkg PCDs related to initial AP bringup and general AP management.
-- 
2.16.2.windows.1

[PATCH 4/5] ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack

[Jian J Wang]

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116

Since PcdSetNxForStack is expired, remove all references. The default
setting of PcdDxeNxMemoryProtectionPolicy has covered PcdSetNxForStack.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Star Zeng <star.zeng@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>
---
 ArmVirtPkg/ArmVirt.dsc.inc | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 70a0ac4d78..d87ae5a32d 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -383,11 +383,6 @@
   #
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy|0xC000000000007FD1
 
-  #
-  # Enable the non-executable DXE stack. (This gets set up by DxeIpl)
-  #
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|TRUE
-
 [PcdsFixedAtBuild.ARM]
   gEmbeddedTokenSpaceGuid.PcdPrePiCpuMemorySize|40
 
-- 
2.16.2.windows.1

[PATCH 5/5] MdeModulePkg: expire PcdSetNxForStack

[Jian J Wang]

BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116

Since the stack memory is allocated as EfiBootServicesData, its NX protection
can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
in setting related PCDs, PcdSetNxForStack will be expired. Set BIT4 of
PcdDxeNxMemoryProtectionPolicy if NX protection is needed for stack.

Cc: Star Zeng <star.zeng@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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>
---
 MdeModulePkg/MdeModulePkg.dec | 10 +---------
 MdeModulePkg/MdeModulePkg.uni | 10 +---------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 74a699cbb7..b1f208909c 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1320,6 +1320,7 @@
   #
   # NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <BR>
   #       User MUST set the same NX protection for EfiBootServicesData and EfiConventionalMemory. <BR>
+  #       Stack is allocated as type of EfiBootServicesData. Enable NX protection for it will also enable NX protection for stack. <BR>
   #
   # e.g. 0x7FD5 can be used for all memory except Code. <BR>
   # e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. <BR>
@@ -1886,15 +1887,6 @@
   # @Prompt Default Creator Revision for ACPI table creation.
   gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiDefaultCreatorRevision|0x01000013|UINT32|0x30001038
 
-  ## Indicates if to set NX for stack.<BR><BR>
-  #  For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>
-  #  For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require
-  #  IA32 PAE is supported and Execute Disable Bit is available.<BR>
-  #   TRUE  - to set NX for stack.<BR>
-  #   FALSE - Not to set NX for stack.<BR>
-  # @Prompt Set NX for stack.
-  gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f
-
   ## This PCD specifies the PCI-based SD/MMC host controller mmio base address.
   # Define the mmio base address of the pci-based SD/MMC host controller. If there are multiple SD/MMC
   # host controllers, their mmio base addresses are calculated one by one from this base address.
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 080b8a62c0..6b26b21f00 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -339,15 +339,6 @@
 
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSerialRegisterStride_HELP  #language en-US "The number of bytes between registers in serial device.  The default is 1 byte."
 
-#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSetNxForStack_PROMPT  #language en-US "Set NX for stack"
-
-#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdSetNxForStack_HELP  #language en-US "Indicates if to set NX for stack.<BR><BR>"
-                                                                                  "For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>"
-                                                                                  "For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require"
-                                                                                  "IA32 PAE is supported and Execute Disable Bit is available.<BR>"
-                                                                                  "TRUE  - to set NX for stack.<BR>"
-                                                                                  "FALSE - Not to set NX for stack.<BR>"
-
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiS3Enable_PROMPT  #language en-US "ACPI S3 Enable"
 
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiS3Enable_HELP  #language en-US "Indicates if ACPI S3 will be enabled.<BR><BR>"
@@ -1129,6 +1120,7 @@
                                                                                                 "\n"
                                                                                                 "NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <BR>\n"
                                                                                                 "User MUST set the same NX protection for EfiBootServicesData and EfiConventionalMemory. <BR>\n"
+                                                                                                "Stack is allocated as type of EfiBootServicesData. Enable NX protection for it will also enable NX protection for stack. <BR>\n"
                                                                                                 "\n"
                                                                                                 "e.g. 0x7FD5 can be used for all memory except Code. <BR>\n"
                                                                                                 "e.g. 0x7BD4 can be used for all memory except Code and ACPINVS/Reserved. <BR>\n"
-- 
2.16.2.windows.1

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Yao, Jiewen]

Thanks to simply the PCD usage.

Reviewed-by: jiewen.yao@intel.com


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org]
> Sent: Tuesday, September 11, 2018 1:17 PM
> To: edk2-devel@lists.01.org
> Subject: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
> 
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
> 
> Since the stack memory is allocated as EfiBootServicesData, its NX
> protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid
> confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
> 
> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
> 
>  ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14
> +++++++++++---
>  MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
>  MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
>  OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
>  OvmfPkg/OvmfPkgX64.dsc                           |  1 -
>  OvmfPkg/PlatformPei/Platform.c                   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
> 
> --
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Ard Biesheuvel]

On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com> wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since the stack memory is allocated as EfiBootServicesData, its NX protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
>

I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.


> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
>
>  ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
>  MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
>  MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
>  OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
>  OvmfPkg/OvmfPkgX64.dsc                           |  1 -
>  OvmfPkg/PlatformPei/Platform.c                   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
>
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Ni, Ruiyu]

On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
> On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com> wrote:
>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>>
>> Since the stack memory is allocated as EfiBootServicesData, its NX protection
>> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
>> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
>> table entries mapping the stack memory.
>>
> 
> I disagree. This removes the possibility to map EfiBootServicesData as
> executable while still mapping the stack NX. As we all know, an
> executable stack is in a class of its own when it comes to
> exploitability, and should *never* be mapped executable unless in
> highly exceptional cases. Mapping all EfiBootServicesData as
> non-executable may cause backward compatibility problems.
Ard,
Are you saying you want the capability of setting certain range of BS 
data as executable? Why does ARM need such capability?
> 
> In particular, this makes it impossible for AArch64 to populate the
> 1:1 mapping using 64k pages (which is necessary for 52-bit address
> support) and still have a non-executable stack, since
> PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.
> 
> So please disregard these patches.
> 
> 
>> Jian J Wang (5):
>>    MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>>    OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>>    OvmfPkg: expire the use of PcdSetNxForStack
>>    ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>>    MdeModulePkg: expire PcdSetNxForStack
>>
>>   ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
>>   MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
>>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
>>   MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>>   MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
>>   MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
>>   MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
>>   OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
>>   OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
>>   OvmfPkg/OvmfPkgX64.dsc                           |  1 -
>>   OvmfPkg/PlatformPei/Platform.c                   |  1 -
>>   OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
>>   13 files changed, 22 insertions(+), 35 deletions(-)
>>
>> --
>> 2.16.2.windows.1
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 


-- 
Thanks,
Ray

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Ard Biesheuvel]

On 11 September 2018 at 11:13, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
>>
>> On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com> wrote:
>>>
>>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>>>
>>> Since the stack memory is allocated as EfiBootServicesData, its NX
>>> protection
>>> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid
>>> confusing
>>> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If
>>> BIT4
>>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in
>>> page
>>> table entries mapping the stack memory.
>>>
>>
>> I disagree. This removes the possibility to map EfiBootServicesData as
>> executable while still mapping the stack NX. As we all know, an
>> executable stack is in a class of its own when it comes to
>> exploitability, and should *never* be mapped executable unless in
>> highly exceptional cases. Mapping all EfiBootServicesData as
>> non-executable may cause backward compatibility problems.
>
> Ard,
> Are you saying you want the capability of setting certain range of BS data
> as executable? Why does ARM need such capability?
>

No, I am saying that mapping all BS data executable should be a
separate decision from mapping the stack executable: if your platform
cannot support the former (for historical reasons) you will likely
still want the latter.

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Ni, Ruiyu]

> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
> Biesheuvel
> Sent: Wednesday, September 12, 2018 5:03 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>
> Cc: edk2-devel@lists.01.org
> Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
> 
> On 11 September 2018 at 11:13, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
> > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
> >>
> >> On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com> wrote:
> >>>
> >>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
> >>>
> >>> Since the stack memory is allocated as EfiBootServicesData, its NX
> >>> protection can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy.
> >>> To avoid confusing in setting related PCDs, PcdSetNxForStack will be
> >>> expired. Instead, If
> >>> BIT4
> >>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit
> >>> in page table entries mapping the stack memory.
> >>>
> >>
> >> I disagree. This removes the possibility to map EfiBootServicesData
> >> as executable while still mapping the stack NX. As we all know, an
> >> executable stack is in a class of its own when it comes to
> >> exploitability, and should *never* be mapped executable unless in
> >> highly exceptional cases. Mapping all EfiBootServicesData as
> >> non-executable may cause backward compatibility problems.
> >
> > Ard,
> > Are you saying you want the capability of setting certain range of BS
> > data as executable? Why does ARM need such capability?
> >
> 
> No, I am saying that mapping all BS data executable should be a separate
> decision from mapping the stack executable: if your platform cannot support the
> former (for historical reasons) you will likely still want the latter.

Let me try to understand the specific problem in ARM64:
ARM64 uses 64KB page size to support 2^52 memory space. With 4KB page size,
it can only support 2^48 memory space.
But due to the DXE core AllocatePages() implementation, the hard-code 4KB granularity
(defined by UEFI spec) causes the page table protection for BS_DATA/BS_CODE is impossible.
So ARM64 chooses to disable the BS_DATA/BS_CODE protection, but only enable
the stack protection.
Correct?
If so, is changing spec to allow page-size platform configurable a better option?

> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Ard Biesheuvel]

On 12 September 2018 at 02:55, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>
>
>> -----Original Message-----
>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>> Biesheuvel
>> Sent: Wednesday, September 12, 2018 5:03 AM
>> To: Ni, Ruiyu <ruiyu.ni@intel.com>
>> Cc: edk2-devel@lists.01.org
>> Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack
>>
>> On 11 September 2018 at 11:13, Ni, Ruiyu <ruiyu.ni@intel.com> wrote:
>> > On 9/11/2018 4:57 PM, Ard Biesheuvel wrote:
>> >>
>> >> On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com> wrote:
>> >>>
>> >>> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>> >>>
>> >>> Since the stack memory is allocated as EfiBootServicesData, its NX
>> >>> protection can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy.
>> >>> To avoid confusing in setting related PCDs, PcdSetNxForStack will be
>> >>> expired. Instead, If
>> >>> BIT4
>> >>> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit
>> >>> in page table entries mapping the stack memory.
>> >>>
>> >>
>> >> I disagree. This removes the possibility to map EfiBootServicesData
>> >> as executable while still mapping the stack NX. As we all know, an
>> >> executable stack is in a class of its own when it comes to
>> >> exploitability, and should *never* be mapped executable unless in
>> >> highly exceptional cases. Mapping all EfiBootServicesData as
>> >> non-executable may cause backward compatibility problems.
>> >
>> > Ard,
>> > Are you saying you want the capability of setting certain range of BS
>> > data as executable? Why does ARM need such capability?
>> >
>>
>> No, I am saying that mapping all BS data executable should be a separate
>> decision from mapping the stack executable: if your platform cannot support the
>> former (for historical reasons) you will likely still want the latter.
>
> Let me try to understand the specific problem in ARM64:
> ARM64 uses 64KB page size to support 2^52 memory space. With 4KB page size,
> it can only support 2^48 memory space.
> But due to the DXE core AllocatePages() implementation, the hard-code 4KB granularity
> (defined by UEFI spec) causes the page table protection for BS_DATA/BS_CODE is impossible.
> So ARM64 chooses to disable the BS_DATA/BS_CODE protection, but only enable
> the stack protection.
> Correct?
> If so, is changing spec to allow page-size platform configurable a better option?
>

I don't think so. We need to retain compatibility with PE/COFF and
third party UEFI images, so changing the page size is likely to result
in much more pain than it cures.

Re: [PATCH 0/5] expire the use of PcdSetNxForStack

[Wang, Jian J]

Hi Ard,

Sorry for the title which misleads you (I used a wrong word too. Shame!).
The real problem this patch series try to address is the confusion in these
two PCDs, PcdSetNxForStack and PcdDxeNxMemoryProtectionPolicy.
One of them will enable NX feature in cpu but another won’t. And there’s
also functionality overlap between them because stack memory is actually
type of EfiBootServiceData, which is also controlled by BIT4 of
PcdDxeNxMemoryProtectionPolicy.

Regards,
Jian

From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
Sent: Tuesday, September 11, 2018 4:58 PM
To: Wang, Jian J <jian.j.wang@intel.com>; Laszlo Ersek <lersek@redhat.com>; Charles Garcia-Tobin <Charles.Garcia-Tobin@arm.com>; Leif Lindholm <leif.lindholm@linaro.org>
Cc: edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH 0/5] expire the use of PcdSetNxForStack

On 11 September 2018 at 07:16, Jian J Wang <jian.j.wang@intel.com<mailto:jian.j.wang@intel.com>> wrote:
> BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116
>
> Since the stack memory is allocated as EfiBootServicesData, its NX protection
> can be covered by BIT4 of PcdDxeNxMemoryProtectionPolicy. To avoid confusing
> in setting related PCDs, PcdSetNxForStack will be expired. Instead, If BIT4
> of PcdDxeNxMemoryProtectionPolicy is set, the DxeIpl will set NX bit in page
> table entries mapping the stack memory.
>

I disagree. This removes the possibility to map EfiBootServicesData as
executable while still mapping the stack NX. As we all know, an
executable stack is in a class of its own when it comes to
exploitability, and should *never* be mapped executable unless in
highly exceptional cases. Mapping all EfiBootServicesData as
non-executable may cause backward compatibility problems.

In particular, this makes it impossible for AArch64 to populate the
1:1 mapping using 64k pages (which is necessary for 52-bit address
support) and still have a non-executable stack, since
PcdDxeNxMemoryProtectionPolicy is disabled in that scenario.

So please disregard these patches.


> Jian J Wang (5):
>   MdeModulePkg/DxeIplPeim: expire the use of PcdSetNxForStack
>   OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack
>   OvmfPkg: expire the use of PcdSetNxForStack
>   ArmVirtPkg/ArmVirt.dsc.inc: expire the use of PcdSetNxForStack
>   MdeModulePkg: expire PcdSetNxForStack
>
>  ArmVirtPkg/ArmVirt.dsc.inc                       |  5 -----
>  MdeModulePkg/Core/DxeIplPeim/Arm/DxeLoadFunc.c   |  6 +++++-
>  MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  3 ++-
>  MdeModulePkg/Core/DxeIplPeim/X64/DxeLoadFunc.c   |  2 +-
>  MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 14 +++++++++++---
>  MdeModulePkg/MdeModulePkg.dec                    | 10 +---------
>  MdeModulePkg/MdeModulePkg.uni                    | 10 +---------
>  OvmfPkg/OvmfPkgIa32.dsc                          |  1 -
>  OvmfPkg/OvmfPkgIa32X64.dsc                       |  1 -
>  OvmfPkg/OvmfPkgX64.dsc                           |  1 -
>  OvmfPkg/PlatformPei/Platform.c                   |  1 -
>  OvmfPkg/PlatformPei/PlatformPei.inf              |  1 -
>  13 files changed, 22 insertions(+), 35 deletions(-)
>
> --
> 2.16.2.windows.1
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>
> https://lists.01.org/mailman/listinfo/edk2-devel