[PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Hao Wu]

V2 changes:

A. Use Hoblib APIs to get the base of stack from Hob.
B. Remove unnecessary local variable used in function
   InitializeDxeNxMemoryProtectionPolicy().

V1 history:

If enabled, NX memory protection feature will mark some types of active
memory as NX (non-executable), which includes the first page of the stack.
This will overwrite the attributes of the first page of the stack if the
stack guard feature is also enabled.

The series will override the attributes setting to the first page of the
stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard
feature is enabled.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>

Hao Wu (2):
  MdeModulePkg/Core: Refine handling NULL detection in NX setting
  MdeModulePkg/Core: Fix feature conflict between NX and Stack guard

 MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 +++++++++++++++++++++++----
 2 files changed, 67 insertions(+), 11 deletions(-)

-- 
2.12.0.windows.1

[PATCH v2 1/2] MdeModulePkg/Core: Refine handling NULL detection in NX setting

[Hao Wu]

The commit rewrites the logic in function
InitializeDxeNxMemoryProtectionPolicy() for handling the first page
(page 0) when NULL pointer detection feature is enabled.

Instead of skip setting the page 0, the codes will now override the
attribute setting of page 0 by adding the 'EFI_MEMORY_RP' attribute.

The purpose is to make it easy for other special handlings of pages
(e.g. the first page of the stack when stack guard feature is enabled).

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 455ed35f9a..a2ea445eef 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -19,7 +19,7 @@
 
   Once the image is unloaded, the protection is removed automatically.
 
-Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR>
 This program and the accompanying materials
 are licensed and made available under the terms and conditions of the BSD License
 which accompanies this distribution.  The full text of the license may be found at
@@ -846,23 +846,23 @@ InitializeDxeNxMemoryProtectionPolicy (
 
     Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
     if (Attributes != 0) {
+      SetUefiImageMemoryAttributes (
+        MemoryMapEntry->PhysicalStart,
+        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
+        Attributes);
+
       if (MemoryMapEntry->PhysicalStart == 0 &&
           PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
 
         ASSERT (MemoryMapEntry->NumberOfPages > 0);
         //
-        // Skip page 0 if NULL pointer detection is enabled to avoid attributes
-        // overwritten.
+        // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer detection is
+        // enabled.
         //
         SetUefiImageMemoryAttributes (
-          MemoryMapEntry->PhysicalStart + EFI_PAGE_SIZE,
-          LShiftU64 (MemoryMapEntry->NumberOfPages - 1, EFI_PAGE_SHIFT),
-          Attributes);
-      } else {
-        SetUefiImageMemoryAttributes (
-          MemoryMapEntry->PhysicalStart,
-          LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT),
-          Attributes);
+          0,
+          EFI_PAGES_TO_SIZE (1),
+          EFI_MEMORY_RP | Attributes);
       }
     }
     MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
-- 
2.12.0.windows.1

[PATCH v2 2/2] MdeModulePkg/Core: Fix feature conflict between NX and Stack guard

[Hao Wu]

If enabled, NX memory protection feature will mark some types of active
memory as NX (non-executable), which includes the first page of the stack.
This will overwrite the attributes of the first page of the stack if the
stack guard feature is also enabled.

The solution is to override the attributes setting to the first page of
the stack by adding back the 'EFI_MEMORY_RP' attribute when the stack
guard feature is enabled.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Hao Wu <hao.a.wu@intel.com>
---
 MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 54 +++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 7334780326..d2e7360ed4 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -3,7 +3,7 @@
 #
 #  It provides an implementation of DXE Core that is compliant with DXE CIS.
 #  
-#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
 #  which accompanies this distribution.  The full text of the license may be found at
@@ -130,6 +130,7 @@
   gEfiPropertiesTableGuid                       ## SOMETIMES_PRODUCES   ## SystemTable
   gEfiMemoryAttributesTableGuid                 ## SOMETIMES_PRODUCES   ## SystemTable
   gEfiEndOfDxeEventGroupGuid                    ## SOMETIMES_CONSUMES   ## Event
+  gEfiHobMemoryAllocStackGuid                   ## SOMETIMES_CONSUMES   ## SystemTable
 
 [Ppis]
   gEfiVectorHandoffInfoPpiGuid                  ## UNDEFINED # HOB
@@ -198,6 +199,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType                       ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType                       ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask                   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard                           ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index a2ea445eef..f3e62dd2c5 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -801,6 +801,9 @@ InitializeDxeNxMemoryProtectionPolicy (
   UINT64                            Attributes;
   LIST_ENTRY                        *Link;
   EFI_GCD_MAP_ENTRY                 *Entry;
+  EFI_PEI_HOB_POINTERS              Hob;
+  EFI_HOB_MEMORY_ALLOCATION         *MemoryHob;
+  EFI_PHYSICAL_ADDRESS              StackBase;
 
   //
   // Get the EFI memory map.
@@ -832,6 +835,40 @@ InitializeDxeNxMemoryProtectionPolicy (
   } while (Status == EFI_BUFFER_TOO_SMALL);
   ASSERT_EFI_ERROR (Status);
 
+  StackBase = 0;
+  if (PcdGetBool (PcdCpuStackGuard)) {
+    //
+    // Get the base of stack from Hob.
+    //
+    Hob.Raw = GetHobList ();
+    while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION, Hob.Raw)) != NULL) {
+      MemoryHob = Hob.MemoryAllocation;
+      if (CompareGuid(&gEfiHobMemoryAllocStackGuid, &MemoryHob->AllocDescriptor.Name)) {
+        DEBUG ((
+          DEBUG_INFO,
+          "%a: StackBase = 0x%016lx  StackSize = 0x%016lx\n",
+          __FUNCTION__,
+          MemoryHob->AllocDescriptor.MemoryBaseAddress,
+          MemoryHob->AllocDescriptor.MemoryLength
+          ));
+
+        StackBase = MemoryHob->AllocDescriptor.MemoryBaseAddress;
+        //
+        // Ensure the base of the stack is page-size aligned.
+        //
+        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
+        break;
+      }
+      Hob.Raw = GET_NEXT_HOB (Hob);
+    }
+
+    //
+    // Ensure the base of stack can be found from Hob when stack guard is
+    // enabled.
+    //
+    ASSERT (StackBase != 0);
+  }
+
   DEBUG ((
     DEBUG_INFO,
     "%a: applying strict permissions to active memory regions\n",
@@ -864,6 +901,23 @@ InitializeDxeNxMemoryProtectionPolicy (
           EFI_PAGES_TO_SIZE (1),
           EFI_MEMORY_RP | Attributes);
       }
+
+      if (StackBase != 0 &&
+          (StackBase >= MemoryMapEntry->PhysicalStart &&
+           StackBase <  MemoryMapEntry->PhysicalStart +
+                        LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT)) &&
+          PcdGetBool (PcdCpuStackGuard)) {
+
+        //
+        // Add EFI_MEMORY_RP attribute for the first page of the stack if stack
+        // guard is enabled.
+        //
+        SetUefiImageMemoryAttributes (
+          StackBase,
+          EFI_PAGES_TO_SIZE (1),
+          EFI_MEMORY_RP | Attributes);
+      }
+
     }
     MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
   }
-- 
2.12.0.windows.1

Re: [PATCH v2 2/2] MdeModulePkg/Core: Fix feature conflict between NX and Stack guard

[Yao, Jiewen]

Both patches are reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, March 6, 2018 9:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH v2 2/2] MdeModulePkg/Core: Fix feature conflict between NX
> and Stack guard
> 
> If enabled, NX memory protection feature will mark some types of active
> memory as NX (non-executable), which includes the first page of the stack.
> This will overwrite the attributes of the first page of the stack if the
> stack guard feature is also enabled.
> 
> The solution is to override the attributes setting to the first page of
> the stack by adding back the 'EFI_MEMORY_RP' attribute when the stack
> guard feature is enabled.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Hao Wu <hao.a.wu@intel.com>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 54
> +++++++++++++++++++++++++++
>  2 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 7334780326..d2e7360ed4 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -3,7 +3,7 @@
>  #
>  #  It provides an implementation of DXE Core that is compliant with DXE CIS.
>  #
> -#  Copyright (c) 2006 - 2017, Intel Corporation. All rights reserved.<BR>
> +#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
>  #  This program and the accompanying materials
>  #  are licensed and made available under the terms and conditions of the BSD
> License
>  #  which accompanies this distribution.  The full text of the license may be
> found at
> @@ -130,6 +130,7 @@
>    gEfiPropertiesTableGuid                       ##
> SOMETIMES_PRODUCES   ## SystemTable
>    gEfiMemoryAttributesTableGuid                 ##
> SOMETIMES_PRODUCES   ## SystemTable
>    gEfiEndOfDxeEventGroupGuid                    ##
> SOMETIMES_CONSUMES   ## Event
> +  gEfiHobMemoryAllocStackGuid                   ##
> SOMETIMES_CONSUMES   ## SystemTable
> 
>  [Ppis]
>    gEfiVectorHandoffInfoPpiGuid                  ## UNDEFINED # HOB
> @@ -198,6 +199,7 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPageType
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPoolType
> ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdHeapGuardPropertyMask
> ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard
> ## CONSUMES
> 
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> index a2ea445eef..f3e62dd2c5 100644
> --- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> +++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
> @@ -801,6 +801,9 @@ InitializeDxeNxMemoryProtectionPolicy (
>    UINT64                            Attributes;
>    LIST_ENTRY                        *Link;
>    EFI_GCD_MAP_ENTRY                 *Entry;
> +  EFI_PEI_HOB_POINTERS              Hob;
> +  EFI_HOB_MEMORY_ALLOCATION         *MemoryHob;
> +  EFI_PHYSICAL_ADDRESS              StackBase;
> 
>    //
>    // Get the EFI memory map.
> @@ -832,6 +835,40 @@ InitializeDxeNxMemoryProtectionPolicy (
>    } while (Status == EFI_BUFFER_TOO_SMALL);
>    ASSERT_EFI_ERROR (Status);
> 
> +  StackBase = 0;
> +  if (PcdGetBool (PcdCpuStackGuard)) {
> +    //
> +    // Get the base of stack from Hob.
> +    //
> +    Hob.Raw = GetHobList ();
> +    while ((Hob.Raw = GetNextHob (EFI_HOB_TYPE_MEMORY_ALLOCATION,
> Hob.Raw)) != NULL) {
> +      MemoryHob = Hob.MemoryAllocation;
> +      if (CompareGuid(&gEfiHobMemoryAllocStackGuid,
> &MemoryHob->AllocDescriptor.Name)) {
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "%a: StackBase = 0x%016lx  StackSize = 0x%016lx\n",
> +          __FUNCTION__,
> +          MemoryHob->AllocDescriptor.MemoryBaseAddress,
> +          MemoryHob->AllocDescriptor.MemoryLength
> +          ));
> +
> +        StackBase = MemoryHob->AllocDescriptor.MemoryBaseAddress;
> +        //
> +        // Ensure the base of the stack is page-size aligned.
> +        //
> +        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
> +        break;
> +      }
> +      Hob.Raw = GET_NEXT_HOB (Hob);
> +    }
> +
> +    //
> +    // Ensure the base of stack can be found from Hob when stack guard is
> +    // enabled.
> +    //
> +    ASSERT (StackBase != 0);
> +  }
> +
>    DEBUG ((
>      DEBUG_INFO,
>      "%a: applying strict permissions to active memory regions\n",
> @@ -864,6 +901,23 @@ InitializeDxeNxMemoryProtectionPolicy (
>            EFI_PAGES_TO_SIZE (1),
>            EFI_MEMORY_RP | Attributes);
>        }
> +
> +      if (StackBase != 0 &&
> +          (StackBase >= MemoryMapEntry->PhysicalStart &&
> +           StackBase <  MemoryMapEntry->PhysicalStart +
> +                        LShiftU64 (MemoryMapEntry->NumberOfPages,
> EFI_PAGE_SHIFT)) &&
> +          PcdGetBool (PcdCpuStackGuard)) {
> +
> +        //
> +        // Add EFI_MEMORY_RP attribute for the first page of the stack if
> stack
> +        // guard is enabled.
> +        //
> +        SetUefiImageMemoryAttributes (
> +          StackBase,
> +          EFI_PAGES_TO_SIZE (1),
> +          EFI_MEMORY_RP | Attributes);
> +      }
> +
>      }
>      MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
> DescriptorSize);
>    }
> --
> 2.12.0.windows.1

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Wang, Jian J]

Thanks for fixing the issue.

For this patch series:

Reviewed-by: Jian J Wang <jian.j.wang@intel.com>

> -----Original Message-----
> From: Wu, Hao A
> Sent: Tuesday, March 06, 2018 9:33 PM
> To: edk2-devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Wang, Jian J <jian.j.wang@intel.com>;
> Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>
> Subject: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
> 
> V2 changes:
> 
> A. Use Hoblib APIs to get the base of stack from Hob.
> B. Remove unnecessary local variable used in function
>    InitializeDxeNxMemoryProtectionPolicy().
> 
> V1 history:
> 
> If enabled, NX memory protection feature will mark some types of active
> memory as NX (non-executable), which includes the first page of the stack.
> This will overwrite the attributes of the first page of the stack if the
> stack guard feature is also enabled.
> 
> The series will override the attributes setting to the first page of the
> stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard
> feature is enabled.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Hao Wu (2):
>   MdeModulePkg/Core: Refine handling NULL detection in NX setting
>   MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
> 
>  MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
> +++++++++++++++++++++++----
>  2 files changed, 67 insertions(+), 11 deletions(-)
> 
> --
> 2.12.0.windows.1

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Ni, Ruiyu]

On 3/6/2018 9:33 PM, Hao Wu wrote:
> V2 changes:
> 
> A. Use Hoblib APIs to get the base of stack from Hob.
> B. Remove unnecessary local variable used in function
>     InitializeDxeNxMemoryProtectionPolicy().
> 
> V1 history:
> 
> If enabled, NX memory protection feature will mark some types of active
> memory as NX (non-executable), which includes the first page of the stack.
> This will overwrite the attributes of the first page of the stack if the
> stack guard feature is also enabled.
> 
> The series will override the attributes setting to the first page of the
> stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard
> feature is enabled.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> 
> Hao Wu (2):
>    MdeModulePkg/Core: Refine handling NULL detection in NX setting
>    MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
> 
>   MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
>   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74 +++++++++++++++++++++++----
>   2 files changed, 67 insertions(+), 11 deletions(-)
> 

       if (MemoryMapEntry->PhysicalStart == 0 &&
           PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {

         ASSERT (MemoryMapEntry->NumberOfPages > 0);
         //
         // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer 
detection is
         // enabled.
         //
[Ray] 1. I prefer to move the above comments before the "if (...)".

         SetUefiImageMemoryAttributes (
           0,
           EFI_PAGES_TO_SIZE (1),
           EFI_MEMORY_RP | Attributes);
       }

       if (StackBase != 0 &&
           (StackBase >= MemoryMapEntry->PhysicalStart &&
            StackBase <  MemoryMapEntry->PhysicalStart +
                         LShiftU64 (MemoryMapEntry->NumberOfPages, 
EFI_PAGE_SHIFT)) &&
           PcdGetBool (PcdCpuStackGuard)) {

         //
         // Add EFI_MEMORY_RP attribute for the first page of the stack 
if stack
         // guard is enabled.
         //
         SetUefiImageMemoryAttributes (
           StackBase,
           EFI_PAGES_TO_SIZE (1),
           EFI_MEMORY_RP | Attributes);
[Ray] 2. The StackBase is directly used here. So do we need to check
whether it's page aligned? Do we need to check whether the range
[StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
       }

-- 
Thanks,
Ray

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Wang, Jian J]

Regards,
Jian


> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Wednesday, March 07, 2018 11:17 AM
> To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
> guard
> 
> On 3/6/2018 9:33 PM, Hao Wu wrote:
> > V2 changes:
> >
> > A. Use Hoblib APIs to get the base of stack from Hob.
> > B. Remove unnecessary local variable used in function
> >     InitializeDxeNxMemoryProtectionPolicy().
> >
> > V1 history:
> >
> > If enabled, NX memory protection feature will mark some types of active
> > memory as NX (non-executable), which includes the first page of the stack.
> > This will overwrite the attributes of the first page of the stack if the
> > stack guard feature is also enabled.
> >
> > The series will override the attributes setting to the first page of the
> > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard
> > feature is enabled.
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> >
> > Hao Wu (2):
> >    MdeModulePkg/Core: Refine handling NULL detection in NX setting
> >    MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
> >
> >   MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
> >   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
> +++++++++++++++++++++++----
> >   2 files changed, 67 insertions(+), 11 deletions(-)
> >
> 
>        if (MemoryMapEntry->PhysicalStart == 0 &&
>            PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> 
>          ASSERT (MemoryMapEntry->NumberOfPages > 0);
>          //
>          // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer
> detection is
>          // enabled.
>          //
> [Ray] 1. I prefer to move the above comments before the "if (...)".
> 
>          SetUefiImageMemoryAttributes (
>            0,
>            EFI_PAGES_TO_SIZE (1),
>            EFI_MEMORY_RP | Attributes);
>        }
> 
>        if (StackBase != 0 &&
>            (StackBase >= MemoryMapEntry->PhysicalStart &&
>             StackBase <  MemoryMapEntry->PhysicalStart +
>                          LShiftU64 (MemoryMapEntry->NumberOfPages,
> EFI_PAGE_SHIFT)) &&
>            PcdGetBool (PcdCpuStackGuard)) {
> 
>          //
>          // Add EFI_MEMORY_RP attribute for the first page of the stack
> if stack
>          // guard is enabled.
>          //
>          SetUefiImageMemoryAttributes (
>            StackBase,
>            EFI_PAGES_TO_SIZE (1),
>            EFI_MEMORY_RP | Attributes);
> [Ray] 2. The StackBase is directly used here. So do we need to check
> whether it's page aligned? Do we need to check whether the range
> [StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
>        }

If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for StackBase
should make sure all the conditions you mentioned, but not here.

> 
> --
> Thanks,
> Ray

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Yao, Jiewen]

I think the original patch is fine.

StackBase is already checked by using ASSERT before.
> +        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);

MemMap entry must be page aligned.

No additional check is required here.

Thank you
Yao Jiewen


> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, March 7, 2018 11:40 AM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> edk2-devel@lists.01.org
> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>
> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard
> 
> 
> 
> Regards,
> Jian
> 
> 
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Wednesday, March 07, 2018 11:17 AM
> > To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star <star.zeng@intel.com>;
> > Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
> > guard
> >
> > On 3/6/2018 9:33 PM, Hao Wu wrote:
> > > V2 changes:
> > >
> > > A. Use Hoblib APIs to get the base of stack from Hob.
> > > B. Remove unnecessary local variable used in function
> > >     InitializeDxeNxMemoryProtectionPolicy().
> > >
> > > V1 history:
> > >
> > > If enabled, NX memory protection feature will mark some types of active
> > > memory as NX (non-executable), which includes the first page of the stack.
> > > This will overwrite the attributes of the first page of the stack if the
> > > stack guard feature is also enabled.
> > >
> > > The series will override the attributes setting to the first page of the
> > > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack guard
> > > feature is enabled.
> > >
> > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > >
> > > Hao Wu (2):
> > >    MdeModulePkg/Core: Refine handling NULL detection in NX setting
> > >    MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
> > >
> > >   MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
> > >   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
> > +++++++++++++++++++++++----
> > >   2 files changed, 67 insertions(+), 11 deletions(-)
> > >
> >
> >        if (MemoryMapEntry->PhysicalStart == 0 &&
> >            PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> >
> >          ASSERT (MemoryMapEntry->NumberOfPages > 0);
> >          //
> >          // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer
> > detection is
> >          // enabled.
> >          //
> > [Ray] 1. I prefer to move the above comments before the "if (...)".
> >
> >          SetUefiImageMemoryAttributes (
> >            0,
> >            EFI_PAGES_TO_SIZE (1),
> >            EFI_MEMORY_RP | Attributes);
> >        }
> >
> >        if (StackBase != 0 &&
> >            (StackBase >= MemoryMapEntry->PhysicalStart &&
> >             StackBase <  MemoryMapEntry->PhysicalStart +
> >                          LShiftU64
> (MemoryMapEntry->NumberOfPages,
> > EFI_PAGE_SHIFT)) &&
> >            PcdGetBool (PcdCpuStackGuard)) {
> >
> >          //
> >          // Add EFI_MEMORY_RP attribute for the first page of the stack
> > if stack
> >          // guard is enabled.
> >          //
> >          SetUefiImageMemoryAttributes (
> >            StackBase,
> >            EFI_PAGES_TO_SIZE (1),
> >            EFI_MEMORY_RP | Attributes);
> > [Ray] 2. The StackBase is directly used here. So do we need to check
> > whether it's page aligned? Do we need to check whether the range
> > [StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
> >        }
> 
> If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for
> StackBase
> should make sure all the conditions you mentioned, but not here.
> 
> >
> > --
> > Thanks,
> > Ray

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Wu, Hao A]

Hi Ray,

Below are the answers to your feedbacks:

> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, March 07, 2018 12:14 PM
> To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org
> Cc: Zeng, Star; Dong, Eric
> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
> guard
> 
> I think the original patch is fine.
> 
> StackBase is already checked by using ASSERT before.
> > +        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
> 
> MemMap entry must be page aligned.
> 
> No additional check is required here.
> 
> Thank you
> Yao Jiewen
> 
> 
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Wednesday, March 7, 2018 11:40 AM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> > edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>
> > Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
> guard
> >
> >
> >
> > Regards,
> > Jian
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ruiyu
> > > Sent: Wednesday, March 07, 2018 11:17 AM
> > > To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star
> <star.zeng@intel.com>;
> > > Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > > Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
> > > guard
> > >
> > > On 3/6/2018 9:33 PM, Hao Wu wrote:
> > > > V2 changes:
> > > >
> > > > A. Use Hoblib APIs to get the base of stack from Hob.
> > > > B. Remove unnecessary local variable used in function
> > > >     InitializeDxeNxMemoryProtectionPolicy().
> > > >
> > > > V1 history:
> > > >
> > > > If enabled, NX memory protection feature will mark some types of active
> > > > memory as NX (non-executable), which includes the first page of the stack.
> > > > This will overwrite the attributes of the first page of the stack if the
> > > > stack guard feature is also enabled.
> > > >
> > > > The series will override the attributes setting to the first page of the
> > > > stack by adding back the 'EFI_MEMORY_RP' attribute when the stack
> guard
> > > > feature is enabled.
> > > >
> > > > Cc: Jian J Wang <jian.j.wang@intel.com>
> > > > Cc: Star Zeng <star.zeng@intel.com>
> > > > Cc: Eric Dong <eric.dong@intel.com>
> > > > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > > >
> > > > Hao Wu (2):
> > > >    MdeModulePkg/Core: Refine handling NULL detection in NX setting
> > > >    MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
> > > >
> > > >   MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
> > > >   MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
> > > +++++++++++++++++++++++----
> > > >   2 files changed, 67 insertions(+), 11 deletions(-)
> > > >
> > >
> > >        if (MemoryMapEntry->PhysicalStart == 0 &&
> > >            PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
> > >
> > >          ASSERT (MemoryMapEntry->NumberOfPages > 0);
> > >          //
> > >          // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer
> > > detection is
> > >          // enabled.
> > >          //
> > > [Ray] 1. I prefer to move the above comments before the "if (...)".

Yes. I agree that moving the comments block out of the 'if' statement is
more readable. I will update according to your suggestion when I pushing
the changes.

> > >
> > >          SetUefiImageMemoryAttributes (
> > >            0,
> > >            EFI_PAGES_TO_SIZE (1),
> > >            EFI_MEMORY_RP | Attributes);
> > >        }
> > >
> > >        if (StackBase != 0 &&
> > >            (StackBase >= MemoryMapEntry->PhysicalStart &&
> > >             StackBase <  MemoryMapEntry->PhysicalStart +
> > >                          LShiftU64
> > (MemoryMapEntry->NumberOfPages,
> > > EFI_PAGE_SHIFT)) &&
> > >            PcdGetBool (PcdCpuStackGuard)) {
> > >
> > >          //
> > >          // Add EFI_MEMORY_RP attribute for the first page of the stack
> > > if stack
> > >          // guard is enabled.
> > >          //
> > >          SetUefiImageMemoryAttributes (
> > >            StackBase,
> > >            EFI_PAGES_TO_SIZE (1),
> > >            EFI_MEMORY_RP | Attributes);
> > > [Ray] 2. The StackBase is directly used here. So do we need to check
> > > whether it's page aligned? Do we need to check whether the range
> > > [StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
> > >        }
> >
> > If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for
> > StackBase
> > should make sure all the conditions you mentioned, but not here.
> >

Just as Jiewen mentioned in the previous reply. An ASSERT:
ASSERT ((StackBase & EFI_PAGE_MASK) == 0);

is added to ensure the stack base fetched from Hob is page-size aligned.

And the below check:
(StackBase >= MemoryMapEntry->PhysicalStart &&
 StackBase <  MemoryMapEntry->PhysicalStart +
              LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT))

together with the fact that MemMap is page aligned (also mentioned by
Jiewen) ensures that the first page of the stack is cover by the memory
range of the MemMap.

Best Regards,
Hao Wu

> > >
> > > --
> > > Thanks,
> > > Ray

Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack guard

[Ni, Ruiyu]

On 3/7/2018 12:28 PM, Wu, Hao A wrote:
> Hi Ray,
> 
> Below are the answers to your feedbacks:
> 
>> -----Original Message-----
>> From: Yao, Jiewen
>> Sent: Wednesday, March 07, 2018 12:14 PM
>> To: Wang, Jian J; Ni, Ruiyu; Wu, Hao A; edk2-devel@lists.01.org
>> Cc: Zeng, Star; Dong, Eric
>> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
>> guard
>>
>> I think the original patch is fine.
>>
>> StackBase is already checked by using ASSERT before.
>>> +        ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
>>
>> MemMap entry must be page aligned.
>>
>> No additional check is required here.
>>
>> Thank you
>> Yao Jiewen
>>
>>
>>> -----Original Message-----
>>> From: Wang, Jian J
>>> Sent: Wednesday, March 7, 2018 11:40 AM
>>> To: Ni, Ruiyu <ruiyu.ni@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
>>> edk2-devel@lists.01.org
>>> Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao,
>>> Jiewen <jiewen.yao@intel.com>
>>> Subject: RE: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
>> guard
>>>
>>>
>>>
>>> Regards,
>>> Jian
>>>
>>>
>>>> -----Original Message-----
>>>> From: Ni, Ruiyu
>>>> Sent: Wednesday, March 07, 2018 11:17 AM
>>>> To: Wu, Hao A <hao.a.wu@intel.com>; edk2-devel@lists.01.org
>>>> Cc: Wang, Jian J <jian.j.wang@intel.com>; Zeng, Star
>> <star.zeng@intel.com>;
>>>> Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
>>>> Subject: Re: [PATCH v2 0/2] Resolve feature conflict between NX and Stack
>>>> guard
>>>>
>>>> On 3/6/2018 9:33 PM, Hao Wu wrote:
>>>>> V2 changes:
>>>>>
>>>>> A. Use Hoblib APIs to get the base of stack from Hob.
>>>>> B. Remove unnecessary local variable used in function
>>>>>      InitializeDxeNxMemoryProtectionPolicy().
>>>>>
>>>>> V1 history:
>>>>>
>>>>> If enabled, NX memory protection feature will mark some types of active
>>>>> memory as NX (non-executable), which includes the first page of the stack.
>>>>> This will overwrite the attributes of the first page of the stack if the
>>>>> stack guard feature is also enabled.
>>>>>
>>>>> The series will override the attributes setting to the first page of the
>>>>> stack by adding back the 'EFI_MEMORY_RP' attribute when the stack
>> guard
>>>>> feature is enabled.
>>>>>
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> Cc: Star Zeng <star.zeng@intel.com>
>>>>> Cc: Eric Dong <eric.dong@intel.com>
>>>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
>>>>>
>>>>> Hao Wu (2):
>>>>>     MdeModulePkg/Core: Refine handling NULL detection in NX setting
>>>>>     MdeModulePkg/Core: Fix feature conflict between NX and Stack guard
>>>>>
>>>>>    MdeModulePkg/Core/Dxe/DxeMain.inf             |  4 +-
>>>>>    MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 74
>>>> +++++++++++++++++++++++----
>>>>>    2 files changed, 67 insertions(+), 11 deletions(-)
>>>>>
>>>>
>>>>         if (MemoryMapEntry->PhysicalStart == 0 &&
>>>>             PcdGet8 (PcdNullPointerDetectionPropertyMask) != 0) {
>>>>
>>>>           ASSERT (MemoryMapEntry->NumberOfPages > 0);
>>>>           //
>>>>           // Add EFI_MEMORY_RP attribute for page 0 if NULL pointer
>>>> detection is
>>>>           // enabled.
>>>>           //
>>>> [Ray] 1. I prefer to move the above comments before the "if (...)".
> 
> Yes. I agree that moving the comments block out of the 'if' statement is
> more readable. I will update according to your suggestion when I pushing
> the changes.
> 
>>>>
>>>>           SetUefiImageMemoryAttributes (
>>>>             0,
>>>>             EFI_PAGES_TO_SIZE (1),
>>>>             EFI_MEMORY_RP | Attributes);
>>>>         }
>>>>
>>>>         if (StackBase != 0 &&
>>>>             (StackBase >= MemoryMapEntry->PhysicalStart &&
>>>>              StackBase <  MemoryMapEntry->PhysicalStart +
>>>>                           LShiftU64
>>> (MemoryMapEntry->NumberOfPages,
>>>> EFI_PAGE_SHIFT)) &&
>>>>             PcdGetBool (PcdCpuStackGuard)) {
>>>>
>>>>           //
>>>>           // Add EFI_MEMORY_RP attribute for the first page of the stack
>>>> if stack
>>>>           // guard is enabled.
>>>>           //
>>>>           SetUefiImageMemoryAttributes (
>>>>             StackBase,
>>>>             EFI_PAGES_TO_SIZE (1),
>>>>             EFI_MEMORY_RP | Attributes);
>>>> [Ray] 2. The StackBase is directly used here. So do we need to check
>>>> whether it's page aligned? Do we need to check whether the range
>>>> [StackBase, StackBase + 4KB) is inside the MemoryMapEntry?
>>>>         }
>>>
>>> If PcdCpuStackGuard is TRUE, I think the owner who allocates memory for
>>> StackBase
>>> should make sure all the conditions you mentioned, but not here.
>>>
> 
> Just as Jiewen mentioned in the previous reply. An ASSERT:
> ASSERT ((StackBase & EFI_PAGE_MASK) == 0);
> 
> is added to ensure the stack base fetched from Hob is page-size aligned.
> 
> And the below check:
> (StackBase >= MemoryMapEntry->PhysicalStart &&
>   StackBase <  MemoryMapEntry->PhysicalStart +
>                LShiftU64 (MemoryMapEntry->NumberOfPages, EFI_PAGE_SHIFT))
> 
> together with the fact that MemMap is page aligned (also mentioned by
> Jiewen) ensures that the first page of the stack is cover by the memory
> range of the MemMap.

I agree. Thanks for the explanation.
Please move the comments location when checking in the patch.

> 
> Best Regards,
> Hao Wu
> 
>>>>
>>>> --
>>>> Thanks,
>>>> Ray


-- 
Thanks,
Ray