[PATCH v3 0/6] RFC: increased memory protection

[PATCH v3 0/6] RFC: increased memory protection

[Ard Biesheuvel]

Hello all,

This is a proof of concept implementation that removes all executable
permissions from writable memory regions, which greatly enhances security.
It is based on Jiewen's recent work, which is a step in the right direction,
but still leaves most of memory exploitable due to the default R+W+X
permissions.

The idea is that the implementation of the CPU arch protocol goes over the
memory map and removes exec permissions from all regions that are not already
marked as 'code. This requires some preparatory work to ensure that the DxeCore
itself is covered by a BootServicesCode region, not a BootServicesData region.
Exec permissions are re-granted selectively, when the PE/COFF loader allocates
the space for it. Combined with Jiewen's code/data split, this removes all
RWX mapped regions.

Changes since v2:
- added patch to make EBC use EfiBootServicesCode pool allocations for thunks
- redefine PCD according to Jiewen's feedback, including default value
- use sorted memory map and merge adjacent entries with the same policy, to
  prevent unnecessary page table splitting
- ignore policy when executing in SMM
- refactor the logic for managing permission attributes of pool allocations
- added some R-b's

Changes since v1:
- allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have
  the expected memory type (as suggested by Jiewen)
- add patch to inhibit page table updates while syncing the GCD memory space
  map with the page tables
- add PCD to set memory protection policy, which allows the policy for reserved
  and ACPI/NVS memory to be configured separately
- move attribute manipulation into DxeCore page allocation code: this way, we
  should be able to solve the EBC case by allocating BootServicesCode pool
  memory explicitly.

Series can be found here:
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2

Note that to test this properly, the default value of 0 should be changed
to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
regions.

Ard Biesheuvel (6):
  ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
  MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
    images
  MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
  MdeModulePkg/DxeCore: use separate lock for pool allocations
  MdeModulePkg: define PCD for DXE memory protection policy
  MdeModulePkg/DxeCore: implement memory protection policy

 ArmPkg/Drivers/CpuDxe/CpuDxe.c                     |   3 +
 ArmPkg/Drivers/CpuDxe/CpuDxe.h                     |   1 +
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c               |   4 +
 MdeModulePkg/Core/Dxe/DxeMain.h                    |  24 ++
 MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
 MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
 MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306 +++++++++++++++++++-
 MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
 MdeModulePkg/MdeModulePkg.dec                      |  31 ++
 MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
 MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
 MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
 MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
 MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
 MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
 16 files changed, 471 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH v3 1/6] ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()

[Ard Biesheuvel]

To prevent the initial MMU->GCD memory space map synchronization from
stripping permissions attributes [which we cannot use in the GCD memory
space map, unfortunately], implement the same approach as x86, and ignore
SetMemoryAttributes() calls during the time SyncCacheConfig() is in
progress. This is a horrible hack, but is currently the only way we can
implement strict permissions on arbitrary memory regions [as opposed to
PE/COFF text/data sections only]

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
 ArmPkg/Drivers/CpuDxe/CpuDxe.c       | 3 +++
 ArmPkg/Drivers/CpuDxe/CpuDxe.h       | 1 +
 ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c | 4 ++++
 3 files changed, 8 insertions(+)

diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.c b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
index 5aa5b874144a..1955d1dece03 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.c
@@ -17,6 +17,7 @@
 
 #include <Guid/IdleLoopEvent.h>
 
+BOOLEAN                   gIsFlushingGCD;
 
 /**
   This function flushes the range of addresses from Start to Start+Length
@@ -261,7 +262,9 @@ CpuDxeInitialize (
   // and that calls EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes, so this code needs to go
   // after the protocol is installed
   //
+  gIsFlushingGCD = TRUE;
   SyncCacheConfig (&mCpu);
+  gIsFlushingGCD = FALSE;
 
   // If the platform is a MPCore system then install the Configuration Table describing the
   // secondary core states
diff --git a/ArmPkg/Drivers/CpuDxe/CpuDxe.h b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
index a00fc3064362..085e4cab2921 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuDxe.h
+++ b/ArmPkg/Drivers/CpuDxe/CpuDxe.h
@@ -37,6 +37,7 @@
 #include <Protocol/DebugSupportPeriodicCallback.h>
 #include <Protocol/LoadedImage.h>
 
+extern BOOLEAN gIsFlushingGCD;
 
 /**
   This function registers and enables the handler specified by InterruptHandler for a processor
diff --git a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
index ebe593d1c325..6dfec7e55888 100644
--- a/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
+++ b/ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c
@@ -188,6 +188,10 @@ CpuSetMemoryAttributes (
   UINTN       RegionLength;
   UINTN       RegionArmAttributes;
 
+  if (gIsFlushingGCD) {
+    return EFI_SUCCESS;
+  }
+
   if ((BaseAddress & (SIZE_4KB - 1)) != 0) {
     // Minimum granularity is SIZE_4KB (4KB on ARM)
     DEBUG ((EFI_D_PAGE, "CpuSetMemoryAttributes(%lx, %lx, %lx): Minimum ganularity is SIZE_4KB\n", BaseAddress, Length, EfiAttributes));
-- 
2.7.4

[PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images

[Ard Biesheuvel]

Ensure that any memory allocated for PE/COFF images is identifiable as
a boot services code region, so that we know it requires its executable
permissions to be preserved when we tighten mapping permissions later on.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Core/Pei/Image/Image.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/MdeModulePkg/Core/Pei/Image/Image.c b/MdeModulePkg/Core/Pei/Image/Image.c
index d659de8b3e64..8cc9ed93e9b6 100644
--- a/MdeModulePkg/Core/Pei/Image/Image.c
+++ b/MdeModulePkg/Core/Pei/Image/Image.c
@@ -453,12 +453,16 @@ LoadAndRelocatePeCoffImage (
         //
         // The PEIM is not assiged valid address, try to allocate page to load it.
         //
-        ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
+        Status = PeiServicesAllocatePages (EfiBootServicesCode,
+                                           EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
+                                           &ImageContext.ImageAddress);
       }
     } else {
-      ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN) AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
+      Status = PeiServicesAllocatePages (EfiBootServicesCode,
+                                         EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
+                                         &ImageContext.ImageAddress);
     }
-    if (ImageContext.ImageAddress != 0) {
+    if (!EFI_ERROR (Status)) {
       //
       // Adjust the Image Address to make sure it is section alignment.
       //
-- 
2.7.4

Re: [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images

[Gao, Liming]

Ard:
  In line 128,  there is another AllocatePages() to allocate memory to store the code. To be consistent, could you help also update it? 
  
Thanks
Liming
>-----Original Message-----
>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>Biesheuvel
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming
><liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
>Subject: [edk2] [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate
>BootServicesCode memory for PE/COFF images
>
>Ensure that any memory allocated for PE/COFF images is identifiable as
>a boot services code region, so that we know it requires its executable
>permissions to be preserved when we tighten mapping permissions later on.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>---
> MdeModulePkg/Core/Pei/Image/Image.c | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
>b/MdeModulePkg/Core/Pei/Image/Image.c
>index d659de8b3e64..8cc9ed93e9b6 100644
>--- a/MdeModulePkg/Core/Pei/Image/Image.c
>+++ b/MdeModulePkg/Core/Pei/Image/Image.c
>@@ -453,12 +453,16 @@ LoadAndRelocatePeCoffImage (
>         //
>         // The PEIM is not assiged valid address, try to allocate page to load it.
>         //
>-        ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)
>AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>+        Status = PeiServicesAllocatePages (EfiBootServicesCode,
>+                                           EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>+                                           &ImageContext.ImageAddress);
>       }
>     } else {
>-      ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)
>AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>+      Status = PeiServicesAllocatePages (EfiBootServicesCode,
>+                                         EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>+                                         &ImageContext.ImageAddress);
>     }
>-    if (ImageContext.ImageAddress != 0) {
>+    if (!EFI_ERROR (Status)) {
>       //
>       // Adjust the Image Address to make sure it is section alignment.
>       //
>--
>2.7.4
>
>_______________________________________________
>edk2-devel mailing list
>edk2-devel@lists.01.org
>https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF images

[Ard Biesheuvel]

On 27 February 2017 at 06:43, Gao, Liming <liming.gao@intel.com> wrote:
> Ard:
>   In line 128,  there is another AllocatePages() to allocate memory to store the code. To be consistent, could you help also update it?
>

OK

>>-----Original Message-----
>>From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard
>>Biesheuvel
>>Sent: Monday, February 27, 2017 2:30 AM
>>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>>leif.lindholm@linaro.org
>>Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel
>><ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming
>><liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
>>lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
>>Subject: [edk2] [PATCH v3 2/6] MdeModulePkg/PeiCore: allocate
>>BootServicesCode memory for PE/COFF images
>>
>>Ensure that any memory allocated for PE/COFF images is identifiable as
>>a boot services code region, so that we know it requires its executable
>>permissions to be preserved when we tighten mapping permissions later on.
>>
>>Contributed-under: TianoCore Contribution Agreement 1.0
>>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>Reviewed-by: Jiewen Yao <jiewen.yao@intel.com>
>>---
>> MdeModulePkg/Core/Pei/Image/Image.c | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>>diff --git a/MdeModulePkg/Core/Pei/Image/Image.c
>>b/MdeModulePkg/Core/Pei/Image/Image.c
>>index d659de8b3e64..8cc9ed93e9b6 100644
>>--- a/MdeModulePkg/Core/Pei/Image/Image.c
>>+++ b/MdeModulePkg/Core/Pei/Image/Image.c
>>@@ -453,12 +453,16 @@ LoadAndRelocatePeCoffImage (
>>         //
>>         // The PEIM is not assiged valid address, try to allocate page to load it.
>>         //
>>-        ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)
>>AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>>+        Status = PeiServicesAllocatePages (EfiBootServicesCode,
>>+                                           EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>>+                                           &ImageContext.ImageAddress);
>>       }
>>     } else {
>>-      ImageContext.ImageAddress = (EFI_PHYSICAL_ADDRESS)(UINTN)
>>AllocatePages (EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize));
>>+      Status = PeiServicesAllocatePages (EfiBootServicesCode,
>>+                                         EFI_SIZE_TO_PAGES ((UINT32) AlignImageSize),
>>+                                         &ImageContext.ImageAddress);
>>     }
>>-    if (ImageContext.ImageAddress != 0) {
>>+    if (!EFI_ERROR (Status)) {
>>       //
>>       // Adjust the Image Address to make sure it is section alignment.
>>       //
>>--
>>2.7.4
>>
>>_______________________________________________
>>edk2-devel mailing list
>>edk2-devel@lists.01.org
>>https://lists.01.org/mailman/listinfo/edk2-devel

[PATCH v3 3/6] MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks

[Ard Biesheuvel]

The EBC driver emits thunks for native to EBC calls, which are short
instructions sequences that bridge the gap between the native execution
environment and the EBC virtual machine.

Since these thunks are allocated using MemoryAllocationLib::AllocatePool(),
they are emitted into EfiBootServicesData regions, which does not reflect
the nature of these thunks accurately, and interferes with strict memory
protection policies that map data regions non-executable.

So instead, create a new helper EbcAllocatePoolForThunk() that invokes the
AllocatePool() boot services directly to allocate EfiBootServicesCode pool
memory explicitly, and wire up this helper for the various architecture
specific thunk generation routines.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |  2 +-
 MdeModulePkg/Universal/EbcDxe/EbcInt.c             | 23 ++++++++++++++++++++
 MdeModulePkg/Universal/EbcDxe/EbcInt.h             | 14 ++++++++++++
 MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |  2 +-
 MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |  2 +-
 MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |  2 +-
 6 files changed, 41 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
index ade47c4d0622..7c13ce12a38b 100644
--- a/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c
@@ -383,7 +383,7 @@ EbcCreateThunks (
     return EFI_INVALID_PARAMETER;
   }
 
-  InstructionBuffer = AllocatePool (sizeof (EBC_INSTRUCTION_BUFFER));
+  InstructionBuffer = EbcAllocatePoolForThunk (sizeof (EBC_INSTRUCTION_BUFFER));
   if (InstructionBuffer == NULL) {
     return EFI_OUT_OF_RESOURCES;
   }
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.c b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
index 6fd2aaf5af27..727ba8bcae44 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.c
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.c
@@ -1410,3 +1410,26 @@ EbcVmTestUnsupported (
   return EFI_UNSUPPORTED;
 }
 
+/**
+  Allocates a buffer of type EfiBootServicesCode.
+
+  @param  AllocationSize        The number of bytes to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EbcAllocatePoolForThunk (
+  IN UINTN  AllocationSize
+  )
+{
+  VOID        *Buffer;
+  EFI_STATUS  Status;
+
+  Status = gBS->AllocatePool (EfiBootServicesCode, AllocationSize, &Buffer);
+  if (EFI_ERROR (Status)) {
+    return NULL;
+  }
+  return Buffer;
+}
diff --git a/MdeModulePkg/Universal/EbcDxe/EbcInt.h b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
index 75017a23e75e..8aa7a4abbd63 100644
--- a/MdeModulePkg/Universal/EbcDxe/EbcInt.h
+++ b/MdeModulePkg/Universal/EbcDxe/EbcInt.h
@@ -246,4 +246,18 @@ typedef struct {
       CR(a, EBC_PROTOCOL_PRIVATE_DATA, EbcProtocol, EBC_PROTOCOL_PRIVATE_DATA_SIGNATURE)
 
 
+/**
+  Allocates a buffer of type EfiBootServicesCode.
+
+  @param  AllocationSize        The number of bytes to allocate.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+VOID *
+EFIAPI
+EbcAllocatePoolForThunk (
+  IN UINTN  AllocationSize
+  );
+
 #endif // #ifndef _EBC_INT_H_
diff --git a/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
index 8e660b93ad64..a825846f89c3 100644
--- a/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c
@@ -484,7 +484,7 @@ EbcCreateThunks (
 
   ThunkSize = sizeof(mInstructionBufferTemplate);
 
-  Ptr = AllocatePool (sizeof(mInstructionBufferTemplate));
+  Ptr = EbcAllocatePoolForThunk (sizeof(mInstructionBufferTemplate));
 
   if (Ptr == NULL) {
     return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
index 95837cb67865..f99348f181a9 100644
--- a/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c
@@ -403,7 +403,7 @@ EbcCreateThunks (
   //
   Size      = EBC_THUNK_SIZE + EBC_THUNK_ALIGNMENT - 1;
   ThunkSize = Size;
-  Ptr = AllocatePool (Size);
+  Ptr = EbcAllocatePoolForThunk (Size);
 
   if (Ptr == NULL) {
     return EFI_OUT_OF_RESOURCES;
diff --git a/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c b/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
index 4325e2e52710..33a174917b69 100644
--- a/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
+++ b/MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c
@@ -441,7 +441,7 @@ EbcCreateThunks (
 
   ThunkSize = sizeof(mInstructionBufferTemplate);
 
-  Ptr = AllocatePool (sizeof(mInstructionBufferTemplate));
+  Ptr = EbcAllocatePoolForThunk (sizeof(mInstructionBufferTemplate));
 
   if (Ptr == NULL) {
     return EFI_OUT_OF_RESOURCES;
-- 
2.7.4

[PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel]

In preparation of adding memory permission attribute management to
the pool allocator, split off the locking of the pool metadata into
a separate lock. This is an improvement in itself, given that pool
allocation can only interfere with the page allocation bookkeeping
if pool pages are allocated or released. But it is also required to
ensure that the permission attribute management does not deadlock,
given that it may trigger page table splits leading to additional
page tables being allocated.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..410615e0dee9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include "DxeMain.h"
 #include "Imem.h"
 
+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
+
 #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
 typedef struct {
   UINT32          Signature;
@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
   //
   // Acquire the memory lock and make the allocation
   //
-  Status = CoreAcquireLockOrFail (&gMemoryLock);
+  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
   if (EFI_ERROR (Status)) {
     return EFI_OUT_OF_RESOURCES;
   }
 
   *Buffer = CoreAllocatePoolI (PoolType, Size);
-  CoreReleaseMemoryLock ();
+  CoreReleaseLock (&mPoolMemoryLock);
   return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
 }
 
@@ -289,6 +291,23 @@ CoreAllocatePool (
   return Status;
 }
 
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+  IN EFI_MEMORY_TYPE    PoolType,
+  IN UINTN              NoPages,
+  IN UINTN              Granularity
+  )
+{
+  VOID    *Buffer;
+
+  CoreAcquireMemoryLock ();
+  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+  CoreReleaseMemoryLock ();
+
+  return Buffer;
+}
+
 /**
   Internal function to allocate pool of a particular type.
   Caller must have the memory lock held
@@ -317,7 +336,7 @@ CoreAllocatePoolI (
   UINTN       NoPages;
   UINTN       Granularity;
 
-  ASSERT_LOCKED (&gMemoryLock);
+  ASSERT_LOCKED (&mPoolMemoryLock);
 
   if  (PoolType == EfiACPIReclaimMemory   ||
        PoolType == EfiACPIMemoryNVS       ||
@@ -355,7 +374,7 @@ CoreAllocatePoolI (
   if (Index >= SIZE_TO_LIST (Granularity)) {
     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
-    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
     goto Done;
   }
 
@@ -383,7 +402,7 @@ CoreAllocatePoolI (
     //
     // Get another page
     //
-    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
+    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
     if (NewPage == NULL) {
       goto Done;
     }
@@ -486,9 +505,9 @@ CoreInternalFreePool (
     return EFI_INVALID_PARAMETER;
   }
 
-  CoreAcquireMemoryLock ();
+  CoreAcquireLock (&mPoolMemoryLock);
   Status = CoreFreePoolI (Buffer, PoolType);
-  CoreReleaseMemoryLock ();
+  CoreReleaseLock (&mPoolMemoryLock);
   return Status;
 }
 
@@ -525,6 +544,19 @@ CoreFreePool (
   return Status;
 }
 
+STATIC
+VOID
+CoreFreePoolPagesI (
+  IN EFI_MEMORY_TYPE        PoolType,
+  IN EFI_PHYSICAL_ADDRESS   Memory,
+  IN UINTN                  NoPages
+  )
+{
+  CoreAcquireMemoryLock ();
+  CoreFreePoolPages (Memory, NoPages);
+  CoreReleaseMemoryLock ();
+}
+
 /**
   Internal function to free a pool entry.
   Caller must have the memory lock held
@@ -573,7 +605,7 @@ CoreFreePoolI (
   //
   ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
   ASSERT (Head->Size == Tail->Size);
-  ASSERT_LOCKED (&gMemoryLock);
+  ASSERT_LOCKED (&mPoolMemoryLock);
 
   if (Tail->Signature != POOL_TAIL_SIGNATURE) {
     return EFI_INVALID_PARAMETER;
@@ -624,7 +656,7 @@ CoreFreePoolI (
     //
     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
-    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
+    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
 
   } else {
 
@@ -680,7 +712,8 @@ CoreFreePoolI (
         //
         // Free the page
         //
-        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
+        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
+          EFI_SIZE_TO_PAGES (Granularity));
       }
     }
   }
-- 
2.7.4

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel]

On 26 February 2017 at 18:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> In preparation of adding memory permission attribute management to
> the pool allocator, split off the locking of the pool metadata into
> a separate lock. This is an improvement in itself, given that pool
> allocation can only interfere with the page allocation bookkeeping
> if pool pages are allocated or released. But it is also required to
> ensure that the permission attribute management does not deadlock,
> given that it may trigger page table splits leading to additional
> page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include "DxeMain.h"
>  #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTIFY);
> +
>  #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
>  typedef struct {
>    UINT32          Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>    //
>    // Acquire the memory lock and make the allocation
>    //
> -  Status = CoreAcquireLockOrFail (&gMemoryLock);
> +  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>
>    *Buffer = CoreAllocatePoolI (PoolType, Size);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
>  }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
>    return Status;
>  }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> +  IN EFI_MEMORY_TYPE    PoolType,
> +  IN UINTN              NoPages,
> +  IN UINTN              Granularity
> +  )
> +{
> +  VOID    *Buffer;
> +
> +  CoreAcquireMemoryLock ();

This should probably be

  EFI_STATUS  Status;

  Status = CoreAcquireLockOrFail (&gMemoryLock);
  if (EFI_ERROR (Status)) {
    return NULL;
  }

to preserve the old behavior.

> +  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +  CoreReleaseMemoryLock ();
> +
> +  return Buffer;
> +}
> +
>  /**
>    Internal function to allocate pool of a particular type.
>    Caller must have the memory lock held
> @@ -317,7 +336,7 @@ CoreAllocatePoolI (
>    UINTN       NoPages;
>    UINTN       Granularity;
>
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if  (PoolType == EfiACPIReclaimMemory   ||
>         PoolType == EfiACPIMemoryNVS       ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
>    if (Index >= SIZE_TO_LIST (Granularity)) {
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>      goto Done;
>    }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
>      //
>      // Get another page
>      //
> -    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> +    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
>      if (NewPage == NULL) {
>        goto Done;
>      }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireMemoryLock ();
> +  CoreAcquireLock (&mPoolMemoryLock);
>    Status = CoreFreePoolI (Buffer, PoolType);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return Status;
>  }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
>    return Status;
>  }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> +  IN EFI_MEMORY_TYPE        PoolType,
> +  IN EFI_PHYSICAL_ADDRESS   Memory,
> +  IN UINTN                  NoPages
> +  )
> +{
> +  CoreAcquireMemoryLock ();
> +  CoreFreePoolPages (Memory, NoPages);
> +  CoreReleaseMemoryLock ();
> +}
> +
>  /**
>    Internal function to free a pool entry.
>    Caller must have the memory lock held
> @@ -573,7 +605,7 @@ CoreFreePoolI (
>    //
>    ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>    ASSERT (Head->Size == Tail->Size);
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>      return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,7 @@ CoreFreePoolI (
>      //
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
> +    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>
>    } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
>          //
>          // Free the page
>          //
> -        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> +        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> +          EFI_SIZE_TO_PAGES (Granularity));
>        }
>      }
>    }
> --
> 2.7.4
>

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Zeng, Star]

Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, February 27, 2017 2:30 AM
To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
 1 file changed, 43 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 7afd2d312c1d..410615e0dee9 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include "DxeMain.h"
 #include "Imem.h"
 
+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE 
+(TPL_NOTIFY);
+
 #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
 typedef struct {
   UINT32          Signature;
@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
   //
   // Acquire the memory lock and make the allocation
   //
-  Status = CoreAcquireLockOrFail (&gMemoryLock);
+  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
   if (EFI_ERROR (Status)) {
     return EFI_OUT_OF_RESOURCES;
   }
 
   *Buffer = CoreAllocatePoolI (PoolType, Size);
-  CoreReleaseMemoryLock ();
+  CoreReleaseLock (&mPoolMemoryLock);
   return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;  }
 
@@ -289,6 +291,23 @@ CoreAllocatePool (
   return Status;
 }
 
+STATIC
+VOID *
+CoreAllocatePoolPagesI (
+  IN EFI_MEMORY_TYPE    PoolType,
+  IN UINTN              NoPages,
+  IN UINTN              Granularity
+  )
+{
+  VOID    *Buffer;
+
+  CoreAcquireMemoryLock ();
+  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);  
+ CoreReleaseMemoryLock ();
+
+  return Buffer;
+}
+
 /**
   Internal function to allocate pool of a particular type.
   Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
   UINTN       NoPages;
   UINTN       Granularity;
 
-  ASSERT_LOCKED (&gMemoryLock);
+  ASSERT_LOCKED (&mPoolMemoryLock);
 
   if  (PoolType == EfiACPIReclaimMemory   ||
        PoolType == EfiACPIMemoryNVS       ||
@@ -355,7 +374,7 @@ CoreAllocatePoolI (
   if (Index >= SIZE_TO_LIST (Granularity)) {
     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
-    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
+    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
     goto Done;
   }
 
@@ -383,7 +402,7 @@ CoreAllocatePoolI (
     //
     // Get another page
     //
-    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
+    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES 
+ (Granularity), Granularity);
     if (NewPage == NULL) {
       goto Done;
     }
@@ -486,9 +505,9 @@ CoreInternalFreePool (
     return EFI_INVALID_PARAMETER;
   }
 
-  CoreAcquireMemoryLock ();
+  CoreAcquireLock (&mPoolMemoryLock);
   Status = CoreFreePoolI (Buffer, PoolType);
-  CoreReleaseMemoryLock ();
+  CoreReleaseLock (&mPoolMemoryLock);
   return Status;
 }
 
@@ -525,6 +544,19 @@ CoreFreePool (
   return Status;
 }
 
+STATIC
+VOID
+CoreFreePoolPagesI (
+  IN EFI_MEMORY_TYPE        PoolType,
+  IN EFI_PHYSICAL_ADDRESS   Memory,
+  IN UINTN                  NoPages
+  )
+{
+  CoreAcquireMemoryLock ();
+  CoreFreePoolPages (Memory, NoPages);
+  CoreReleaseMemoryLock ();
+}
+
 /**
   Internal function to free a pool entry.
   Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
   //
   ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
   ASSERT (Head->Size == Tail->Size);
-  ASSERT_LOCKED (&gMemoryLock);
+  ASSERT_LOCKED (&mPoolMemoryLock);
 
   if (Tail->Signature != POOL_TAIL_SIGNATURE) {
     return EFI_INVALID_PARAMETER;
@@ -624,7 +656,7 @@ CoreFreePoolI (
     //
     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
-    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
+    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) 
+ (UINTN) Head, NoPages);
 
   } else {
 
@@ -680,7 +712,8 @@ CoreFreePoolI (
         //
         // Free the page
         //
-        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
+        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
+          EFI_SIZE_TO_PAGES (Granularity));
       }
     }
   }
--
2.7.4

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

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel]

On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>

I need it after patch 6. But perhaps it is better to only add it
there, not here.


> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
> Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star <star.zeng@intel.com>
> Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
>
> In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include "DxeMain.h"
>  #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
> +(TPL_NOTIFY);
> +
>  #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
>  typedef struct {
>    UINT32          Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>    //
>    // Acquire the memory lock and make the allocation
>    //
> -  Status = CoreAcquireLockOrFail (&gMemoryLock);
> +  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>
>    *Buffer = CoreAllocatePoolI (PoolType, Size);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;  }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
>    return Status;
>  }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> +  IN EFI_MEMORY_TYPE    PoolType,
> +  IN UINTN              NoPages,
> +  IN UINTN              Granularity
> +  )
> +{
> +  VOID    *Buffer;
> +
> +  CoreAcquireMemoryLock ();
> +  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> + CoreReleaseMemoryLock ();
> +
> +  return Buffer;
> +}
> +
>  /**
>    Internal function to allocate pool of a particular type.
>    Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
>    UINTN       NoPages;
>    UINTN       Granularity;
>
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if  (PoolType == EfiACPIReclaimMemory   ||
>         PoolType == EfiACPIMemoryNVS       ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
>    if (Index >= SIZE_TO_LIST (Granularity)) {
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>      goto Done;
>    }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
>      //
>      // Get another page
>      //
> -    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> +    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
> + (Granularity), Granularity);
>      if (NewPage == NULL) {
>        goto Done;
>      }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireMemoryLock ();
> +  CoreAcquireLock (&mPoolMemoryLock);
>    Status = CoreFreePoolI (Buffer, PoolType);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return Status;
>  }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
>    return Status;
>  }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> +  IN EFI_MEMORY_TYPE        PoolType,
> +  IN EFI_PHYSICAL_ADDRESS   Memory,
> +  IN UINTN                  NoPages
> +  )
> +{
> +  CoreAcquireMemoryLock ();
> +  CoreFreePoolPages (Memory, NoPages);
> +  CoreReleaseMemoryLock ();
> +}
> +
>  /**
>    Internal function to free a pool entry.
>    Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
>    //
>    ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>    ASSERT (Head->Size == Tail->Size);
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>      return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,7 @@ CoreFreePoolI (
>      //
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
> +    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
> + (UINTN) Head, NoPages);
>
>    } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
>          //
>          // Free the page
>          //
> -        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> +        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> +          EFI_SIZE_TO_PAGES (Granularity));
>        }
>      }
>    }
> --
> 2.7.4
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Zeng, Star]

If it is really needed, I am fine to either this patch or another patch to add it. :)

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Ard Biesheuvel
Sent: Monday, February 27, 2017 4:16 PM
To: Zeng, Star <star.zeng@intel.com>
Cc: Tian, Feng <feng.tian@intel.com>; edk2-devel@lists.01.org; afish@apple.com; Gao, Liming <liming.gao@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com
Subject: Re: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

On 27 February 2017 at 01:56, Zeng, Star <star.zeng@intel.com> wrote:
> Minor comment: CoreFreePoolPagesI() has no need to have PoolType parameter, how about to remove it?
>

I need it after patch 6. But perhaps it is better to only add it there, not here.


> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of 
> Ard Biesheuvel
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; 
> leif.lindholm@linaro.org
> Cc: Tian, Feng <feng.tian@intel.com>; Ard Biesheuvel 
> <ard.biesheuvel@linaro.org>; afish@apple.com; Gao, Liming 
> <liming.gao@intel.com>; Kinney, Michael D 
> <michael.d.kinney@intel.com>; lersek@redhat.com; Zeng, Star 
> <star.zeng@intel.com>
> Subject: [edk2] [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock 
> for pool allocations
>
> In preparation of adding memory permission attribute management to the pool allocator, split off the locking of the pool metadata into a separate lock. This is an improvement in itself, given that pool allocation can only interfere with the page allocation bookkeeping if pool pages are allocated or released. But it is also required to ensure that the permission attribute management does not deadlock, given that it may trigger page table splits leading to additional page tables being allocated.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>  1 file changed, 43 insertions(+), 10 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c 
> b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> index 7afd2d312c1d..410615e0dee9 100644
> --- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
> +++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
> @@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include "DxeMain.h"
>  #include "Imem.h"
>
> +STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE 
> +(TPL_NOTIFY);
> +
>  #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
>  typedef struct {
>    UINT32          Signature;
> @@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>    //
>    // Acquire the memory lock and make the allocation
>    //
> -  Status = CoreAcquireLockOrFail (&gMemoryLock);
> +  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>    if (EFI_ERROR (Status)) {
>      return EFI_OUT_OF_RESOURCES;
>    }
>
>    *Buffer = CoreAllocatePoolI (PoolType, Size);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;  }
>
> @@ -289,6 +291,23 @@ CoreAllocatePool (
>    return Status;
>  }
>
> +STATIC
> +VOID *
> +CoreAllocatePoolPagesI (
> +  IN EFI_MEMORY_TYPE    PoolType,
> +  IN UINTN              NoPages,
> +  IN UINTN              Granularity
> +  )
> +{
> +  VOID    *Buffer;
> +
> +  CoreAcquireMemoryLock ();
> +  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity); 
> + CoreReleaseMemoryLock ();
> +
> +  return Buffer;
> +}
> +
>  /**
>    Internal function to allocate pool of a particular type.
>    Caller must have the memory lock held @@ -317,7 +336,7 @@ CoreAllocatePoolI (
>    UINTN       NoPages;
>    UINTN       Granularity;
>
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if  (PoolType == EfiACPIReclaimMemory   ||
>         PoolType == EfiACPIMemoryNVS       ||
> @@ -355,7 +374,7 @@ CoreAllocatePoolI (
>    if (Index >= SIZE_TO_LIST (Granularity)) {
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
> +    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>      goto Done;
>    }
>
> @@ -383,7 +402,7 @@ CoreAllocatePoolI (
>      //
>      // Get another page
>      //
> -    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES (Granularity), Granularity);
> +    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES 
> + (Granularity), Granularity);
>      if (NewPage == NULL) {
>        goto Done;
>      }
> @@ -486,9 +505,9 @@ CoreInternalFreePool (
>      return EFI_INVALID_PARAMETER;
>    }
>
> -  CoreAcquireMemoryLock ();
> +  CoreAcquireLock (&mPoolMemoryLock);
>    Status = CoreFreePoolI (Buffer, PoolType);
> -  CoreReleaseMemoryLock ();
> +  CoreReleaseLock (&mPoolMemoryLock);
>    return Status;
>  }
>
> @@ -525,6 +544,19 @@ CoreFreePool (
>    return Status;
>  }
>
> +STATIC
> +VOID
> +CoreFreePoolPagesI (
> +  IN EFI_MEMORY_TYPE        PoolType,
> +  IN EFI_PHYSICAL_ADDRESS   Memory,
> +  IN UINTN                  NoPages
> +  )
> +{
> +  CoreAcquireMemoryLock ();
> +  CoreFreePoolPages (Memory, NoPages);
> +  CoreReleaseMemoryLock ();
> +}
> +
>  /**
>    Internal function to free a pool entry.
>    Caller must have the memory lock held @@ -573,7 +605,7 @@ CoreFreePoolI (
>    //
>    ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>    ASSERT (Head->Size == Tail->Size);
> -  ASSERT_LOCKED (&gMemoryLock);
> +  ASSERT_LOCKED (&mPoolMemoryLock);
>
>    if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>      return EFI_INVALID_PARAMETER;
> @@ -624,7 +656,7 @@ CoreFreePoolI (
>      //
>      NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) - 1;
>      NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
> -    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
> +    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
> + (UINTN) Head, NoPages);
>
>    } else {
>
> @@ -680,7 +712,8 @@ CoreFreePoolI (
>          //
>          // Free the page
>          //
> -        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage, EFI_SIZE_TO_PAGES (Granularity));
> +        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
> +          EFI_SIZE_TO_PAGES (Granularity));
>        }
>      }
>    }
> --
> 2.7.4
>
> _______________________________________________
> 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

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Gao, Liming]

Ard:
  I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI (). 

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
>Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool
>allocations
>
>In preparation of adding memory permission attribute management to
>the pool allocator, split off the locking of the pool metadata into
>a separate lock. This is an improvement in itself, given that pool
>allocation can only interfere with the page allocation bookkeeping
>if pool pages are allocated or released. But it is also required to
>ensure that the permission attribute management does not deadlock,
>given that it may trigger page table splits leading to additional
>page tables being allocated.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 7afd2d312c1d..410615e0dee9 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>KIND, EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>(TPL_NOTIFY);
>+
> #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
> typedef struct {
>   UINT32          Signature;
>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>   //
>   // Acquire the memory lock and make the allocation
>   //
>-  Status = CoreAcquireLockOrFail (&gMemoryLock);
>+  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>   if (EFI_ERROR (Status)) {
>     return EFI_OUT_OF_RESOURCES;
>   }
>
>   *Buffer = CoreAllocatePoolI (PoolType, Size);
>-  CoreReleaseMemoryLock ();
>+  CoreReleaseLock (&mPoolMemoryLock);
>   return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES;
> }
>
>@@ -289,6 +291,23 @@ CoreAllocatePool (
>   return Status;
> }
>
>+STATIC
>+VOID *
>+CoreAllocatePoolPagesI (
>+  IN EFI_MEMORY_TYPE    PoolType,
>+  IN UINTN              NoPages,
>+  IN UINTN              Granularity
>+  )
>+{
>+  VOID    *Buffer;
>+
>+  CoreAcquireMemoryLock ();
>+  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+  CoreReleaseMemoryLock ();
>+
>+  return Buffer;
>+}
>+
> /**
>   Internal function to allocate pool of a particular type.
>   Caller must have the memory lock held
>@@ -317,7 +336,7 @@ CoreAllocatePoolI (
>   UINTN       NoPages;
>   UINTN       Granularity;
>
>-  ASSERT_LOCKED (&gMemoryLock);
>+  ASSERT_LOCKED (&mPoolMemoryLock);
>
>   if  (PoolType == EfiACPIReclaimMemory   ||
>        PoolType == EfiACPIMemoryNVS       ||
>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
>   if (Index >= SIZE_TO_LIST (Granularity)) {
>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) -
>1;
>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>-    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>     goto Done;
>   }
>
>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
>     //
>     // Get another page
>     //
>-    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>+    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>     if (NewPage == NULL) {
>       goto Done;
>     }
>@@ -486,9 +505,9 @@ CoreInternalFreePool (
>     return EFI_INVALID_PARAMETER;
>   }
>
>-  CoreAcquireMemoryLock ();
>+  CoreAcquireLock (&mPoolMemoryLock);
>   Status = CoreFreePoolI (Buffer, PoolType);
>-  CoreReleaseMemoryLock ();
>+  CoreReleaseLock (&mPoolMemoryLock);
>   return Status;
> }
>
>@@ -525,6 +544,19 @@ CoreFreePool (
>   return Status;
> }
>
>+STATIC
>+VOID
>+CoreFreePoolPagesI (
>+  IN EFI_MEMORY_TYPE        PoolType,
>+  IN EFI_PHYSICAL_ADDRESS   Memory,
>+  IN UINTN                  NoPages
>+  )
>+{
>+  CoreAcquireMemoryLock ();
>+  CoreFreePoolPages (Memory, NoPages);
>+  CoreReleaseMemoryLock ();
>+}
>+
> /**
>   Internal function to free a pool entry.
>   Caller must have the memory lock held
>@@ -573,7 +605,7 @@ CoreFreePoolI (
>   //
>   ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>   ASSERT (Head->Size == Tail->Size);
>-  ASSERT_LOCKED (&gMemoryLock);
>+  ASSERT_LOCKED (&mPoolMemoryLock);
>
>   if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>     return EFI_INVALID_PARAMETER;
>@@ -624,7 +656,7 @@ CoreFreePoolI (
>     //
>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES (Granularity) -
>1;
>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>-    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>+    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN) Head, NoPages);
>
>   } else {
>
>@@ -680,7 +712,8 @@ CoreFreePoolI (
>         //
>         // Free the page
>         //
>-        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>EFI_SIZE_TO_PAGES (Granularity));
>+        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN)NewPage,
>+          EFI_SIZE_TO_PAGES (Granularity));
>       }
>     }
>   }
>--
>2.7.4

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Zeng, Star]

CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.

Thanks,
Star 
-----Original Message-----
From: Gao, Liming 
Sent: Monday, February 27, 2017 2:43 PM
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

Ard:
  I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI (). 

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; 
>leif.lindholm@linaro.org
>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; 
>Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng 
><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel 
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for 
>pool allocations
>
>In preparation of adding memory permission attribute management to the 
>pool allocator, split off the locking of the pool metadata into a 
>separate lock. This is an improvement in itself, given that pool 
>allocation can only interfere with the page allocation bookkeeping if 
>pool pages are allocated or released. But it is also required to ensure 
>that the permission attribute management does not deadlock, given that 
>it may trigger page table splits leading to additional page tables 
>being allocated.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
> 1 file changed, 43 insertions(+), 10 deletions(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 7afd2d312c1d..410615e0dee9 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, 
>EITHER EXPRESS OR IMPLIED.
> #include "DxeMain.h"
> #include "Imem.h"
>
>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>(TPL_NOTIFY);
>+
> #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
> typedef struct {
>   UINT32          Signature;
>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>   //
>   // Acquire the memory lock and make the allocation
>   //
>-  Status = CoreAcquireLockOrFail (&gMemoryLock);
>+  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>   if (EFI_ERROR (Status)) {
>     return EFI_OUT_OF_RESOURCES;
>   }
>
>   *Buffer = CoreAllocatePoolI (PoolType, Size);
>-  CoreReleaseMemoryLock ();
>+  CoreReleaseLock (&mPoolMemoryLock);
>   return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>
>@@ -289,6 +291,23 @@ CoreAllocatePool (
>   return Status;
> }
>
>+STATIC
>+VOID *
>+CoreAllocatePoolPagesI (
>+  IN EFI_MEMORY_TYPE    PoolType,
>+  IN UINTN              NoPages,
>+  IN UINTN              Granularity
>+  )
>+{
>+  VOID    *Buffer;
>+
>+  CoreAcquireMemoryLock ();
>+  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);  
>+ CoreReleaseMemoryLock ();
>+
>+  return Buffer;
>+}
>+
> /**
>   Internal function to allocate pool of a particular type.
>   Caller must have the memory lock held @@ -317,7 +336,7 @@ 
>CoreAllocatePoolI (
>   UINTN       NoPages;
>   UINTN       Granularity;
>
>-  ASSERT_LOCKED (&gMemoryLock);
>+  ASSERT_LOCKED (&mPoolMemoryLock);
>
>   if  (PoolType == EfiACPIReclaimMemory   ||
>        PoolType == EfiACPIMemoryNVS       ||
>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
>   if (Index >= SIZE_TO_LIST (Granularity)) {
>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES 
>(Granularity) - 1;
>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>-    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>+    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>     goto Done;
>   }
>
>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
>     //
>     // Get another page
>     //
>-    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>+    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>(Granularity), Granularity);
>     if (NewPage == NULL) {
>       goto Done;
>     }
>@@ -486,9 +505,9 @@ CoreInternalFreePool (
>     return EFI_INVALID_PARAMETER;
>   }
>
>-  CoreAcquireMemoryLock ();
>+  CoreAcquireLock (&mPoolMemoryLock);
>   Status = CoreFreePoolI (Buffer, PoolType);
>-  CoreReleaseMemoryLock ();
>+  CoreReleaseLock (&mPoolMemoryLock);
>   return Status;
> }
>
>@@ -525,6 +544,19 @@ CoreFreePool (
>   return Status;
> }
>
>+STATIC
>+VOID
>+CoreFreePoolPagesI (
>+  IN EFI_MEMORY_TYPE        PoolType,
>+  IN EFI_PHYSICAL_ADDRESS   Memory,
>+  IN UINTN                  NoPages
>+  )
>+{
>+  CoreAcquireMemoryLock ();
>+  CoreFreePoolPages (Memory, NoPages);
>+  CoreReleaseMemoryLock ();
>+}
>+
> /**
>   Internal function to free a pool entry.
>   Caller must have the memory lock held @@ -573,7 +605,7 @@ 
>CoreFreePoolI (
>   //
>   ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>   ASSERT (Head->Size == Tail->Size);
>-  ASSERT_LOCKED (&gMemoryLock);
>+  ASSERT_LOCKED (&mPoolMemoryLock);
>
>   if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>     return EFI_INVALID_PARAMETER;
>@@ -624,7 +656,7 @@ CoreFreePoolI (
>     //
>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES 
>(Granularity) - 1;
>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>-    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>+    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN) Head, NoPages);
>
>   } else {
>
>@@ -680,7 +712,8 @@ CoreFreePoolI (
>         //
>         // Free the page
>         //
>-        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>EFI_SIZE_TO_PAGES (Granularity));
>+        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>(UINTN)NewPage,
>+          EFI_SIZE_TO_PAGES (Granularity));
>       }
>     }
>   }
>--
>2.7.4

Re: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations

[Ard Biesheuvel]

On 27 February 2017 at 06:50, Zeng, Star <star.zeng@intel.com> wrote:
> CoreAllocatePoolPages() could not be updated simply by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(), it is also used by AllocateMemoryMapEntry() with the lock locked.
>

Indeed.

But I am wondering now if that means some code paths don't set the
protection correctly. If EfiBootServicesData and EfiConventionalMemory
use the same policy (which should be the case 99.9% of the time) it
doesn't matter, but if the configured policy is different, the
permissions will go out of sync.

> -----Original Message-----
> From: Gao, Liming
> Sent: Monday, February 27, 2017 2:43 PM
> To: Ard Biesheuvel <ard.biesheuvel@linaro.org>; edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; leif.lindholm@linaro.org
> Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; lersek@redhat.com; Tian, Feng <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for pool allocations
>
> Ard:
>   I agree to separate lock for pool allocations. I suggest you update CoreAllocatePoolPages() and CoreFreePoolPages() implementation by adding CoreAcquireMemoryLock() and CoreReleaseMemoryLock(). If so, you don't need to introduce new CoreAllocatePoolPagesI () and CoreFreePoolPagesI ().
>
> Thanks
> Liming
>>-----Original Message-----
>>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>>Sent: Monday, February 27, 2017 2:30 AM
>>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>>leif.lindholm@linaro.org
>>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>;
>>Gao, Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
>><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
>><ard.biesheuvel@linaro.org>
>>Subject: [PATCH v3 4/6] MdeModulePkg/DxeCore: use separate lock for
>>pool allocations
>>
>>In preparation of adding memory permission attribute management to the
>>pool allocator, split off the locking of the pool metadata into a
>>separate lock. This is an improvement in itself, given that pool
>>allocation can only interfere with the page allocation bookkeeping if
>>pool pages are allocated or released. But it is also required to ensure
>>that the permission attribute management does not deadlock, given that
>>it may trigger page table splits leading to additional page tables
>>being allocated.
>>
>>Contributed-under: TianoCore Contribution Agreement 1.0
>>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>---
>> MdeModulePkg/Core/Dxe/Mem/Pool.c | 53 ++++++++++++++++----
>> 1 file changed, 43 insertions(+), 10 deletions(-)
>>
>>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>index 7afd2d312c1d..410615e0dee9 100644
>>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>>@@ -15,6 +15,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND,
>>EITHER EXPRESS OR IMPLIED.
>> #include "DxeMain.h"
>> #include "Imem.h"
>>
>>+STATIC EFI_LOCK mPoolMemoryLock = EFI_INITIALIZE_LOCK_VARIABLE
>>(TPL_NOTIFY);
>>+
>> #define POOL_FREE_SIGNATURE   SIGNATURE_32('p','f','r','0')
>> typedef struct {
>>   UINT32          Signature;
>>@@ -239,13 +241,13 @@ CoreInternalAllocatePool (
>>   //
>>   // Acquire the memory lock and make the allocation
>>   //
>>-  Status = CoreAcquireLockOrFail (&gMemoryLock);
>>+  Status = CoreAcquireLockOrFail (&mPoolMemoryLock);
>>   if (EFI_ERROR (Status)) {
>>     return EFI_OUT_OF_RESOURCES;
>>   }
>>
>>   *Buffer = CoreAllocatePoolI (PoolType, Size);
>>-  CoreReleaseMemoryLock ();
>>+  CoreReleaseLock (&mPoolMemoryLock);
>>   return (*Buffer != NULL) ? EFI_SUCCESS : EFI_OUT_OF_RESOURCES; }
>>
>>@@ -289,6 +291,23 @@ CoreAllocatePool (
>>   return Status;
>> }
>>
>>+STATIC
>>+VOID *
>>+CoreAllocatePoolPagesI (
>>+  IN EFI_MEMORY_TYPE    PoolType,
>>+  IN UINTN              NoPages,
>>+  IN UINTN              Granularity
>>+  )
>>+{
>>+  VOID    *Buffer;
>>+
>>+  CoreAcquireMemoryLock ();
>>+  Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>>+ CoreReleaseMemoryLock ();
>>+
>>+  return Buffer;
>>+}
>>+
>> /**
>>   Internal function to allocate pool of a particular type.
>>   Caller must have the memory lock held @@ -317,7 +336,7 @@
>>CoreAllocatePoolI (
>>   UINTN       NoPages;
>>   UINTN       Granularity;
>>
>>-  ASSERT_LOCKED (&gMemoryLock);
>>+  ASSERT_LOCKED (&mPoolMemoryLock);
>>
>>   if  (PoolType == EfiACPIReclaimMemory   ||
>>        PoolType == EfiACPIMemoryNVS       ||
>>@@ -355,7 +374,7 @@ CoreAllocatePoolI (
>>   if (Index >= SIZE_TO_LIST (Granularity)) {
>>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES
>>(Granularity) - 1;
>>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>>-    Head = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>>+    Head = CoreAllocatePoolPagesI (PoolType, NoPages, Granularity);
>>     goto Done;
>>   }
>>
>>@@ -383,7 +402,7 @@ CoreAllocatePoolI (
>>     //
>>     // Get another page
>>     //
>>-    NewPage = CoreAllocatePoolPages(PoolType, EFI_SIZE_TO_PAGES
>>(Granularity), Granularity);
>>+    NewPage = CoreAllocatePoolPagesI (PoolType, EFI_SIZE_TO_PAGES
>>(Granularity), Granularity);
>>     if (NewPage == NULL) {
>>       goto Done;
>>     }
>>@@ -486,9 +505,9 @@ CoreInternalFreePool (
>>     return EFI_INVALID_PARAMETER;
>>   }
>>
>>-  CoreAcquireMemoryLock ();
>>+  CoreAcquireLock (&mPoolMemoryLock);
>>   Status = CoreFreePoolI (Buffer, PoolType);
>>-  CoreReleaseMemoryLock ();
>>+  CoreReleaseLock (&mPoolMemoryLock);
>>   return Status;
>> }
>>
>>@@ -525,6 +544,19 @@ CoreFreePool (
>>   return Status;
>> }
>>
>>+STATIC
>>+VOID
>>+CoreFreePoolPagesI (
>>+  IN EFI_MEMORY_TYPE        PoolType,
>>+  IN EFI_PHYSICAL_ADDRESS   Memory,
>>+  IN UINTN                  NoPages
>>+  )
>>+{
>>+  CoreAcquireMemoryLock ();
>>+  CoreFreePoolPages (Memory, NoPages);
>>+  CoreReleaseMemoryLock ();
>>+}
>>+
>> /**
>>   Internal function to free a pool entry.
>>   Caller must have the memory lock held @@ -573,7 +605,7 @@
>>CoreFreePoolI (
>>   //
>>   ASSERT (Tail->Signature == POOL_TAIL_SIGNATURE);
>>   ASSERT (Head->Size == Tail->Size);
>>-  ASSERT_LOCKED (&gMemoryLock);
>>+  ASSERT_LOCKED (&mPoolMemoryLock);
>>
>>   if (Tail->Signature != POOL_TAIL_SIGNATURE) {
>>     return EFI_INVALID_PARAMETER;
>>@@ -624,7 +656,7 @@ CoreFreePoolI (
>>     //
>>     NoPages = EFI_SIZE_TO_PAGES(Size) + EFI_SIZE_TO_PAGES
>>(Granularity) - 1;
>>     NoPages &= ~(UINTN)(EFI_SIZE_TO_PAGES (Granularity) - 1);
>>-    CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN) Head, NoPages);
>>+    CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>>(UINTN) Head, NoPages);
>>
>>   } else {
>>
>>@@ -680,7 +712,8 @@ CoreFreePoolI (
>>         //
>>         // Free the page
>>         //
>>-        CoreFreePoolPages ((EFI_PHYSICAL_ADDRESS) (UINTN)NewPage,
>>EFI_SIZE_TO_PAGES (Granularity));
>>+        CoreFreePoolPagesI (Pool->MemoryType, (EFI_PHYSICAL_ADDRESS)
>>(UINTN)NewPage,
>>+          EFI_SIZE_TO_PAGES (Granularity));
>>       }
>>     }
>>   }
>>--
>>2.7.4
>

[PATCH v3 5/6] MdeModulePkg: define PCD for DXE memory protection policy

[Ard Biesheuvel]

Define a new fixed/patchable PCD that sets the DXE memory protection
policy: its primary use is to define which memory types should have
their executable permissions removed. Combined with the image protection
policy, this can be used to implement a strict W^X policy, i.e.. a policy
where no regions exist that are both executable and writable at the same
time.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/MdeModulePkg.dec | 31 ++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 426634fbbd4d..fb6a39c9e354 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1107,6 +1107,37 @@ [PcdsFixedAtBuild, PcdsPatchableInModule]
   # @ValidRange 0x80000002 | 0x00000000 - 0x0000001F
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x00000002|UINT32|0x00001047
 
+  ## Set DXE memory protection policy. The policy is bitwise.
+  #  If a bit is set, memory regions of the associated type will be mapped
+  #  non-executable.<BR><BR>
+  #
+  # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
+  #  EfiReservedMemoryType          0x0001<BR>
+  #  EfiLoaderCode                  0x0002<BR>
+  #  EfiLoaderData                  0x0004<BR>
+  #  EfiBootServicesCode            0x0008<BR>
+  #  EfiBootServicesData            0x0010<BR>
+  #  EfiRuntimeServicesCode         0x0020<BR>
+  #  EfiRuntimeServicesData         0x0040<BR>
+  #  EfiConventionalMemory          0x0080<BR>
+  #  EfiUnusableMemory              0x0100<BR>
+  #  EfiACPIReclaimMemory           0x0200<BR>
+  #  EfiACPIMemoryNVS               0x0400<BR>
+  #  EfiMemoryMappedIO              0x0800<BR>
+  #  EfiMemoryMappedIOPortSpace     0x1000<BR>
+  #  EfiPalCode                     0x2000<BR>
+  #  EfiPersistentMemory            0x4000<BR>
+  #  OEM Reserved       0x4000000000000000<BR>
+  #  OS Reserved        0x8000000000000000<BR>
+  #
+  # NOTE: User must NOT set NX protection for EfiLoaderCode / EfiBootServicesCode / EfiRuntimeServicesCode. <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>
+  #
+  # @Prompt Set DXE memory protection policy.
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
+
   ## PCI Serial Device Info. It is an array of Device, Function, and Power Management
   #  information that describes the path that contains zero or more PCI to PCI briges
   #  followed by a PCI serial device.  Each array entry is 4-bytes in length.  The
-- 
2.7.4

[PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy

[Ard Biesheuvel]

This implements a DXE memory protection policy that ensure that regions
that don't require executable permissions are mapped with the non-exec
attribute set.

First of all, it iterates over all entries in the UEFI memory map, and
removes executable permissions according to the configured DXE memory
protection policy, as recorded in PcdDxeMemoryProtectionPolicy.

Secondly, it sets or clears the non-executable attribute when allocating
or freeing pages, both for page based or pool based allocations.

Note that this complements the image protection facility, which applies
strict permissions to BootServicesCode/RuntimeServicesCode regions when
the section alignment allows it. The memory protection configured by this
patch operates on non-code regions only.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 MdeModulePkg/Core/Dxe/DxeMain.h               |  24 ++
 MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +
 MdeModulePkg/Core/Dxe/Mem/Page.c              |   4 +
 MdeModulePkg/Core/Dxe/Mem/Pool.c              |   7 +
 MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
 5 files changed, 341 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index b14be9a74d8e..5668c1f2d648 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -2949,4 +2949,28 @@ MemoryProtectionExitBootServicesCallback (
   VOID
   );
 
+/**
+  Manage memory permission attributes on a memory range, according to the
+  configured DXE memory protection policy.
+
+  @param  OldType           The old memory type of the range
+  @param  NewType           The new memory type of the range
+  @param  Memory            The base address of the range
+  @param  Length            The size of the range (in bytes)
+
+  @return EFI_SUCCESS       If the the CPU arch protocol is not installed yet
+  @return EFI_SUCCESS       If no DXE memory protection policy has been configured
+  @return EFI_SUCCESS       If OldType and NewType use the same permission attributes
+  @return other             Return value of gCpu->SetMemoryAttributes()
+
+**/
+EFI_STATUS
+EFIAPI
+ApplyMemoryProtectionPolicy (
+  IN  EFI_MEMORY_TYPE       OldType,
+  IN  EFI_MEMORY_TYPE       NewType,
+  IN  EFI_PHYSICAL_ADDRESS  Memory,
+  IN  UINT64                Length
+  );
+
 #endif
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 371e91cb0d7e..30d5984f7c1f 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -191,6 +191,7 @@ [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfileDriverPath                 ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
 
 # [Hob]
 # RESOURCE_DESCRIPTOR   ## CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c b/MdeModulePkg/Core/Dxe/Mem/Page.c
index bda4f6397e91..86874906de58 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
@@ -1344,6 +1344,8 @@ CoreAllocatePages (
       NULL
       );
     InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+    ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType, *Memory,
+      EFI_PAGES_TO_SIZE (NumberOfPages));
   }
   return Status;
 }
@@ -1460,6 +1462,8 @@ CoreFreePages (
       NULL
       );
     InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
+    ApplyMemoryProtectionPolicy (MemoryType, EfiConventionalMemory, Memory,
+      EFI_PAGES_TO_SIZE (NumberOfPages));
   }
   return Status;
 }
diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c b/MdeModulePkg/Core/Dxe/Mem/Pool.c
index 410615e0dee9..63b9983b88b5 100644
--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
@@ -305,6 +305,10 @@ CoreAllocatePoolPagesI (
   Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
   CoreReleaseMemoryLock ();
 
+  if (Buffer != NULL) {
+    ApplyMemoryProtectionPolicy (EfiConventionalMemory, PoolType,
+      (EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE (NoPages));
+  }
   return Buffer;
 }
 
@@ -555,6 +559,9 @@ CoreFreePoolPagesI (
   CoreAcquireMemoryLock ();
   CoreFreePoolPages (Memory, NoPages);
   CoreReleaseMemoryLock ();
+
+  ApplyMemoryProtectionPolicy (PoolType, EfiConventionalMemory,
+    (EFI_PHYSICAL_ADDRESS)(UINTN)Memory, EFI_PAGES_TO_SIZE (NoPages));
 }
 
 /**
diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
index 46d88463d417..f2a69a3d0df9 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
@@ -64,6 +64,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define DO_NOT_PROTECT                         0x00000000
 #define PROTECT_IF_ALIGNED_ELSE_ALLOW          0x00000001
 
+#define MEMORY_TYPE_OS_RESERVED_MIN            0x80000000
+#define MEMORY_TYPE_OEM_RESERVED_MIN           0x70000000
+
+#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
+  ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
+
 UINT32   mImageProtectionPolicy;
 
 /**
@@ -647,6 +653,210 @@ UnprotectUefiImage (
 }
 
 /**
+  Return the EFI memory permission attribute associated with memory
+  type 'Type' under the configured DXE memory protection policy.
+**/
+STATIC
+UINT64
+GetPermissionAttributeForMemoryType (
+  IN EFI_MEMORY_TYPE    MemoryType
+  )
+{
+  UINT64 TestBit;
+
+  if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
+    TestBit = BIT63;
+  } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN) {
+    TestBit = BIT62;
+  } else {
+    TestBit = LShiftU64 (1, MemoryType);
+  }
+
+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) != 0) {
+    return EFI_MEMORY_XP;
+  } else {
+    return 0;
+  }
+}
+
+/**
+  Sort memory map entries based upon PhysicalStart, from low to high.
+
+  @param  MemoryMap              A pointer to the buffer in which firmware places
+                                 the current memory map.
+  @param  MemoryMapSize          Size, in bytes, of the MemoryMap buffer.
+  @param  DescriptorSize         Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+STATIC
+VOID
+SortMemoryMap (
+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
+  IN UINTN                      MemoryMapSize,
+  IN UINTN                      DescriptorSize
+  )
+{
+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR       *NextMemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEnd;
+  EFI_MEMORY_DESCRIPTOR       TempMemoryMap;
+
+  MemoryMapEntry = MemoryMap;
+  NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
+  while (MemoryMapEntry < MemoryMapEnd) {
+    while (NextMemoryMapEntry < MemoryMapEnd) {
+      if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry->PhysicalStart) {
+        CopyMem (&TempMemoryMap, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+        CopyMem (MemoryMapEntry, NextMemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+        CopyMem (NextMemoryMapEntry, &TempMemoryMap, sizeof(EFI_MEMORY_DESCRIPTOR));
+      }
+
+      NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+    }
+
+    MemoryMapEntry      = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+    NextMemoryMapEntry  = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+  }
+}
+
+/**
+  Merge adjacent memory map entries if they use the same memory protection policy
+
+  @param[in, out]  MemoryMap              A pointer to the buffer in which firmware places
+                                          the current memory map.
+  @param[in, out]  MemoryMapSize          A pointer to the size, in bytes, of the
+                                          MemoryMap buffer. On input, this is the size of
+                                          the current memory map.  On output,
+                                          it is the size of new memory map after merge.
+  @param[in]       DescriptorSize         Size, in bytes, of an individual EFI_MEMORY_DESCRIPTOR.
+**/
+STATIC
+VOID
+MergeMemoryMapForProtectionPolicy (
+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
+  IN OUT UINTN                  *MemoryMapSize,
+  IN UINTN                      DescriptorSize
+  )
+{
+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEnd;
+  UINT64                      MemoryBlockLength;
+  EFI_MEMORY_DESCRIPTOR       *NewMemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR       *NextMemoryMapEntry;
+  UINT64                      Attributes;
+
+  SortMemoryMap (MemoryMap, *MemoryMapSize, DescriptorSize);
+
+  MemoryMapEntry = MemoryMap;
+  NewMemoryMapEntry = MemoryMap;
+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + *MemoryMapSize);
+  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
+    CopyMem (NewMemoryMapEntry, MemoryMapEntry, sizeof(EFI_MEMORY_DESCRIPTOR));
+    NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+
+    do {
+      MemoryBlockLength = (UINT64) (EFI_PAGES_TO_SIZE((UINTN)MemoryMapEntry->NumberOfPages));
+      Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
+
+      if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
+          Attributes == GetPermissionAttributeForMemoryType (NextMemoryMapEntry->Type) &&
+          ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) == NextMemoryMapEntry->PhysicalStart)) {
+        MemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
+        if (NewMemoryMapEntry != MemoryMapEntry) {
+          NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry->NumberOfPages;
+        }
+
+        NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+        continue;
+      } else {
+        MemoryMapEntry = PREVIOUS_MEMORY_DESCRIPTOR (NextMemoryMapEntry, DescriptorSize);
+        break;
+      }
+    } while (TRUE);
+
+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+    NewMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (NewMemoryMapEntry, DescriptorSize);
+  }
+
+  *MemoryMapSize = (UINTN)NewMemoryMapEntry - (UINTN)MemoryMap;
+
+  return ;
+}
+
+
+/**
+  Remove exec permissions from all regions whose type is identified by
+  PcdDxeNxMemoryProtectionPolicy
+**/
+STATIC
+VOID
+InitializeDxeNxMemoryProtectionPolicy (
+  VOID
+  )
+{
+  UINTN                             MemoryMapSize;
+  UINTN                             MapKey;
+  UINTN                             DescriptorSize;
+  UINT32                            DescriptorVersion;
+  EFI_MEMORY_DESCRIPTOR             *MemoryMap;
+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEntry;
+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEnd;
+  EFI_STATUS                        Status;
+  UINT64                            Attributes;
+
+  //
+  // Get the EFI memory map.
+  //
+  MemoryMapSize = 0;
+  MemoryMap     = NULL;
+
+  Status = gBS->GetMemoryMap (
+                  &MemoryMapSize,
+                  MemoryMap,
+                  &MapKey,
+                  &DescriptorSize,
+                  &DescriptorVersion
+                  );
+  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
+  do {
+    MemoryMap = (EFI_MEMORY_DESCRIPTOR *) AllocatePool (MemoryMapSize);
+    ASSERT (MemoryMap != NULL);
+    Status = gBS->GetMemoryMap (
+                    &MemoryMapSize,
+                    MemoryMap,
+                    &MapKey,
+                    &DescriptorSize,
+                    &DescriptorVersion
+                    );
+    if (EFI_ERROR (Status)) {
+      FreePool (MemoryMap);
+    }
+  } while (Status == EFI_BUFFER_TOO_SMALL);
+  ASSERT_EFI_ERROR (Status);
+
+  DEBUG((DEBUG_ERROR, "%a: removing exec permissions from memory regions\n",
+    __FUNCTION__));
+
+  MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize, DescriptorSize);
+
+  MemoryMapEntry = MemoryMap;
+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap + MemoryMapSize);
+  while ((UINTN) MemoryMapEntry < (UINTN) MemoryMapEnd) {
+
+    Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry->Type);
+    if (Attributes != 0) {
+      SetUefiImageMemoryAttributes (
+        MemoryMapEntry->PhysicalStart,
+        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
+        Attributes);
+    }
+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry, DescriptorSize);
+  }
+  FreePool (MemoryMap);
+}
+
+
+/**
   A notification for CPU_ARCH protocol.
 
   @param[in]  Event                 Event whose notification function is being invoked.
@@ -674,6 +884,17 @@ MemoryProtectionCpuArchProtocolNotify (
     return;
   }
 
+  //
+  // Apply the memory protection policy on non-BScode/RTcode regions.
+  //
+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
+    InitializeDxeNxMemoryProtectionPolicy ();
+  }
+
+  if (mImageProtectionPolicy == 0) {
+    return;
+  }
+
   Status = gBS->LocateHandleBuffer (
                   ByProtocol,
                   &gEfiLoadedImageProtocolGuid,
@@ -753,7 +974,7 @@ CoreInitializeMemoryProtection (
 
   mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
 
-  if (mImageProtectionPolicy != 0) {
+  if (mImageProtectionPolicy != 0 || PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
     Status = CoreCreateEvent (
                EVT_NOTIFY_SIGNAL,
                TPL_CALLBACK,
@@ -775,3 +996,86 @@ CoreInitializeMemoryProtection (
   }
   return ;
 }
+
+STATIC
+BOOLEAN
+IsInSmm (
+  VOID
+  )
+{
+  BOOLEAN     InSmm;
+
+  InSmm = FALSE;
+  if (gSmmBase2 != NULL) {
+    gSmmBase2->InSmm (gSmmBase2, &InSmm);
+  }
+  return InSmm;
+}
+
+/**
+  Manage memory permission attributes on a memory range, according to the
+  configured DXE memory protection policy.
+
+  @param  OldType           The old memory type of the range
+  @param  NewType           The new memory type of the range
+  @param  Memory            The base address of the range
+  @param  Length            The size of the range (in bytes)
+
+  @return EFI_SUCCESS       If we are executing in SMM mode. No permission attributes
+                            are updated in this case
+  @return EFI_SUCCESS       If the the CPU arch protocol is not installed yet
+  @return EFI_SUCCESS       If no DXE memory protection policy has been configured
+  @return EFI_SUCCESS       If OldType and NewType use the same permission attributes
+  @return other             Return value of gCpu->SetMemoryAttributes()
+
+**/
+EFI_STATUS
+EFIAPI
+ApplyMemoryProtectionPolicy (
+  IN  EFI_MEMORY_TYPE       OldType,
+  IN  EFI_MEMORY_TYPE       NewType,
+  IN  EFI_PHYSICAL_ADDRESS  Memory,
+  IN  UINT64                Length
+  )
+{
+  UINT64  OldAttributes;
+  UINT64  NewAttributes;
+
+  //
+  // The policy configured in PcdDxeNxMemoryProtectionPolicy
+  // does not apply to allocations performed in SMM mode.
+  //
+  if (IsInSmm ()) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // If the CPU arch protocol is not installed yet, we cannot manage memory
+  // permission attributes, and it is the job of the driver that installs this
+  // protocol to set the permissions on existing allocations.
+  //
+  if (gCpu == NULL) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Check if a DXE memory protection policy has been configured
+  //
+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) == 0) {
+    return EFI_SUCCESS;
+  }
+
+  //
+  // Update the executable permissions according to the DXE memory
+  // protection policy, but only if the policy is different between
+  // the old and the new type.
+  //
+  OldAttributes = GetPermissionAttributeForMemoryType (OldType);
+  NewAttributes = GetPermissionAttributeForMemoryType (NewType);
+
+  if (OldAttributes == NewAttributes) {
+    return EFI_SUCCESS;
+  }
+
+  return gCpu->SetMemoryAttributes (gCpu, Memory, Length, NewAttributes);
+}
-- 
2.7.4

Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy

[Gao, Liming]

Ard:
  I have minor comment. GetPermissionAttributeForMemoryType() function header comment doesn't match its definition, and IsInSmm() has no function header. 

Thanks
Liming
>-----Original Message-----
>From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
>Sent: Monday, February 27, 2017 2:30 AM
>To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
>leif.lindholm@linaro.org
>Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
>Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
><feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
><ard.biesheuvel@linaro.org>
>Subject: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory
>protection policy
>
>This implements a DXE memory protection policy that ensure that regions
>that don't require executable permissions are mapped with the non-exec
>attribute set.
>
>First of all, it iterates over all entries in the UEFI memory map, and
>removes executable permissions according to the configured DXE memory
>protection policy, as recorded in PcdDxeMemoryProtectionPolicy.
>
>Secondly, it sets or clears the non-executable attribute when allocating
>or freeing pages, both for page based or pool based allocations.
>
>Note that this complements the image protection facility, which applies
>strict permissions to BootServicesCode/RuntimeServicesCode regions when
>the section alignment allows it. The memory protection configured by this
>patch operates on non-code regions only.
>
>Contributed-under: TianoCore Contribution Agreement 1.0
>Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>---
> MdeModulePkg/Core/Dxe/DxeMain.h               |  24 ++
> MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +
> MdeModulePkg/Core/Dxe/Mem/Page.c              |   4 +
> MdeModulePkg/Core/Dxe/Mem/Pool.c              |   7 +
> MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306
>+++++++++++++++++++-
> 5 files changed, 341 insertions(+), 1 deletion(-)
>
>diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
>b/MdeModulePkg/Core/Dxe/DxeMain.h
>index b14be9a74d8e..5668c1f2d648 100644
>--- a/MdeModulePkg/Core/Dxe/DxeMain.h
>+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
>@@ -2949,4 +2949,28 @@ MemoryProtectionExitBootServicesCallback (
>   VOID
>   );
>
>+/**
>+  Manage memory permission attributes on a memory range, according to
>the
>+  configured DXE memory protection policy.
>+
>+  @param  OldType           The old memory type of the range
>+  @param  NewType           The new memory type of the range
>+  @param  Memory            The base address of the range
>+  @param  Length            The size of the range (in bytes)
>+
>+  @return EFI_SUCCESS       If the the CPU arch protocol is not installed yet
>+  @return EFI_SUCCESS       If no DXE memory protection policy has been
>configured
>+  @return EFI_SUCCESS       If OldType and NewType use the same permission
>attributes
>+  @return other             Return value of gCpu->SetMemoryAttributes()
>+
>+**/
>+EFI_STATUS
>+EFIAPI
>+ApplyMemoryProtectionPolicy (
>+  IN  EFI_MEMORY_TYPE       OldType,
>+  IN  EFI_MEMORY_TYPE       NewType,
>+  IN  EFI_PHYSICAL_ADDRESS  Memory,
>+  IN  UINT64                Length
>+  );
>+
> #endif
>diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
>b/MdeModulePkg/Core/Dxe/DxeMain.inf
>index 371e91cb0d7e..30d5984f7c1f 100644
>--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>@@ -191,6 +191,7 @@ [Pcd]
>   gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfileDriverPath
>## CONSUMES
>   gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ##
>CONSUMES
>   gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ##
>CONSUMES
>+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy
>## CONSUMES
>
> # [Hob]
> # RESOURCE_DESCRIPTOR   ## CONSUMES
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Page.c
>b/MdeModulePkg/Core/Dxe/Mem/Page.c
>index bda4f6397e91..86874906de58 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Page.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Page.c
>@@ -1344,6 +1344,8 @@ CoreAllocatePages (
>       NULL
>       );
>     InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
>+    ApplyMemoryProtectionPolicy (EfiConventionalMemory, MemoryType,
>*Memory,
>+      EFI_PAGES_TO_SIZE (NumberOfPages));
>   }
>   return Status;
> }
>@@ -1460,6 +1462,8 @@ CoreFreePages (
>       NULL
>       );
>     InstallMemoryAttributesTableOnMemoryAllocation (MemoryType);
>+    ApplyMemoryProtectionPolicy (MemoryType, EfiConventionalMemory,
>Memory,
>+      EFI_PAGES_TO_SIZE (NumberOfPages));
>   }
>   return Status;
> }
>diff --git a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>index 410615e0dee9..63b9983b88b5 100644
>--- a/MdeModulePkg/Core/Dxe/Mem/Pool.c
>+++ b/MdeModulePkg/Core/Dxe/Mem/Pool.c
>@@ -305,6 +305,10 @@ CoreAllocatePoolPagesI (
>   Buffer = CoreAllocatePoolPages (PoolType, NoPages, Granularity);
>   CoreReleaseMemoryLock ();
>
>+  if (Buffer != NULL) {
>+    ApplyMemoryProtectionPolicy (EfiConventionalMemory, PoolType,
>+      (EFI_PHYSICAL_ADDRESS)(UINTN)Buffer, EFI_PAGES_TO_SIZE
>(NoPages));
>+  }
>   return Buffer;
> }
>
>@@ -555,6 +559,9 @@ CoreFreePoolPagesI (
>   CoreAcquireMemoryLock ();
>   CoreFreePoolPages (Memory, NoPages);
>   CoreReleaseMemoryLock ();
>+
>+  ApplyMemoryProtectionPolicy (PoolType, EfiConventionalMemory,
>+    (EFI_PHYSICAL_ADDRESS)(UINTN)Memory, EFI_PAGES_TO_SIZE
>(NoPages));
> }
>
> /**
>diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>index 46d88463d417..f2a69a3d0df9 100644
>--- a/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c
>@@ -64,6 +64,12 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY
>KIND, EITHER EXPRESS OR IMPLIED.
> #define DO_NOT_PROTECT                         0x00000000
> #define PROTECT_IF_ALIGNED_ELSE_ALLOW          0x00000001
>
>+#define MEMORY_TYPE_OS_RESERVED_MIN            0x80000000
>+#define MEMORY_TYPE_OEM_RESERVED_MIN           0x70000000
>+
>+#define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
>+  ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
>+
> UINT32   mImageProtectionPolicy;
>
> /**
>@@ -647,6 +653,210 @@ UnprotectUefiImage (
> }
>
> /**
>+  Return the EFI memory permission attribute associated with memory
>+  type 'Type' under the configured DXE memory protection policy.
>+**/
>+STATIC
>+UINT64
>+GetPermissionAttributeForMemoryType (
>+  IN EFI_MEMORY_TYPE    MemoryType
>+  )
>+{
>+  UINT64 TestBit;
>+
>+  if ((UINT32) MemoryType >= MEMORY_TYPE_OS_RESERVED_MIN) {
>+    TestBit = BIT63;
>+  } else if ((UINT32) MemoryType >= MEMORY_TYPE_OEM_RESERVED_MIN)
>{
>+    TestBit = BIT62;
>+  } else {
>+    TestBit = LShiftU64 (1, MemoryType);
>+  }
>+
>+  if ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & TestBit) != 0) {
>+    return EFI_MEMORY_XP;
>+  } else {
>+    return 0;
>+  }
>+}
>+
>+/**
>+  Sort memory map entries based upon PhysicalStart, from low to high.
>+
>+  @param  MemoryMap              A pointer to the buffer in which firmware
>places
>+                                 the current memory map.
>+  @param  MemoryMapSize          Size, in bytes, of the MemoryMap buffer.
>+  @param  DescriptorSize         Size, in bytes, of an individual
>EFI_MEMORY_DESCRIPTOR.
>+**/
>+STATIC
>+VOID
>+SortMemoryMap (
>+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
>+  IN UINTN                      MemoryMapSize,
>+  IN UINTN                      DescriptorSize
>+  )
>+{
>+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEntry;
>+  EFI_MEMORY_DESCRIPTOR       *NextMemoryMapEntry;
>+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEnd;
>+  EFI_MEMORY_DESCRIPTOR       TempMemoryMap;
>+
>+  MemoryMapEntry = MemoryMap;
>+  NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ MemoryMapSize);
>+  while (MemoryMapEntry < MemoryMapEnd) {
>+    while (NextMemoryMapEntry < MemoryMapEnd) {
>+      if (MemoryMapEntry->PhysicalStart > NextMemoryMapEntry-
>>PhysicalStart) {
>+        CopyMem (&TempMemoryMap, MemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+        CopyMem (MemoryMapEntry, NextMemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+        CopyMem (NextMemoryMapEntry, &TempMemoryMap,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+      }
>+
>+      NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+    }
>+
>+    MemoryMapEntry      = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+    NextMemoryMapEntry  = NEXT_MEMORY_DESCRIPTOR
>(MemoryMapEntry, DescriptorSize);
>+  }
>+}
>+
>+/**
>+  Merge adjacent memory map entries if they use the same memory
>protection policy
>+
>+  @param[in, out]  MemoryMap              A pointer to the buffer in which
>firmware places
>+                                          the current memory map.
>+  @param[in, out]  MemoryMapSize          A pointer to the size, in bytes, of
>the
>+                                          MemoryMap buffer. On input, this is the size of
>+                                          the current memory map.  On output,
>+                                          it is the size of new memory map after merge.
>+  @param[in]       DescriptorSize         Size, in bytes, of an individual
>EFI_MEMORY_DESCRIPTOR.
>+**/
>+STATIC
>+VOID
>+MergeMemoryMapForProtectionPolicy (
>+  IN OUT EFI_MEMORY_DESCRIPTOR  *MemoryMap,
>+  IN OUT UINTN                  *MemoryMapSize,
>+  IN UINTN                      DescriptorSize
>+  )
>+{
>+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEntry;
>+  EFI_MEMORY_DESCRIPTOR       *MemoryMapEnd;
>+  UINT64                      MemoryBlockLength;
>+  EFI_MEMORY_DESCRIPTOR       *NewMemoryMapEntry;
>+  EFI_MEMORY_DESCRIPTOR       *NextMemoryMapEntry;
>+  UINT64                      Attributes;
>+
>+  SortMemoryMap (MemoryMap, *MemoryMapSize, DescriptorSize);
>+
>+  MemoryMapEntry = MemoryMap;
>+  NewMemoryMapEntry = MemoryMap;
>+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ *MemoryMapSize);
>+  while ((UINTN)MemoryMapEntry < (UINTN)MemoryMapEnd) {
>+    CopyMem (NewMemoryMapEntry, MemoryMapEntry,
>sizeof(EFI_MEMORY_DESCRIPTOR));
>+    NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(MemoryMapEntry, DescriptorSize);
>+
>+    do {
>+      MemoryBlockLength = (UINT64)
>(EFI_PAGES_TO_SIZE((UINTN)MemoryMapEntry->NumberOfPages));
>+      Attributes = GetPermissionAttributeForMemoryType
>(MemoryMapEntry->Type);
>+
>+      if (((UINTN)NextMemoryMapEntry < (UINTN)MemoryMapEnd) &&
>+          Attributes == GetPermissionAttributeForMemoryType
>(NextMemoryMapEntry->Type) &&
>+          ((MemoryMapEntry->PhysicalStart + MemoryBlockLength) ==
>NextMemoryMapEntry->PhysicalStart)) {
>+        MemoryMapEntry->NumberOfPages += NextMemoryMapEntry-
>>NumberOfPages;
>+        if (NewMemoryMapEntry != MemoryMapEntry) {
>+          NewMemoryMapEntry->NumberOfPages += NextMemoryMapEntry-
>>NumberOfPages;
>+        }
>+
>+        NextMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+        continue;
>+      } else {
>+        MemoryMapEntry = PREVIOUS_MEMORY_DESCRIPTOR
>(NextMemoryMapEntry, DescriptorSize);
>+        break;
>+      }
>+    } while (TRUE);
>+
>+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+    NewMemoryMapEntry = NEXT_MEMORY_DESCRIPTOR
>(NewMemoryMapEntry, DescriptorSize);
>+  }
>+
>+  *MemoryMapSize = (UINTN)NewMemoryMapEntry -
>(UINTN)MemoryMap;
>+
>+  return ;
>+}
>+
>+
>+/**
>+  Remove exec permissions from all regions whose type is identified by
>+  PcdDxeNxMemoryProtectionPolicy
>+**/
>+STATIC
>+VOID
>+InitializeDxeNxMemoryProtectionPolicy (
>+  VOID
>+  )
>+{
>+  UINTN                             MemoryMapSize;
>+  UINTN                             MapKey;
>+  UINTN                             DescriptorSize;
>+  UINT32                            DescriptorVersion;
>+  EFI_MEMORY_DESCRIPTOR             *MemoryMap;
>+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEntry;
>+  EFI_MEMORY_DESCRIPTOR             *MemoryMapEnd;
>+  EFI_STATUS                        Status;
>+  UINT64                            Attributes;
>+
>+  //
>+  // Get the EFI memory map.
>+  //
>+  MemoryMapSize = 0;
>+  MemoryMap     = NULL;
>+
>+  Status = gBS->GetMemoryMap (
>+                  &MemoryMapSize,
>+                  MemoryMap,
>+                  &MapKey,
>+                  &DescriptorSize,
>+                  &DescriptorVersion
>+                  );
>+  ASSERT (Status == EFI_BUFFER_TOO_SMALL);
>+  do {
>+    MemoryMap = (EFI_MEMORY_DESCRIPTOR *) AllocatePool
>(MemoryMapSize);
>+    ASSERT (MemoryMap != NULL);
>+    Status = gBS->GetMemoryMap (
>+                    &MemoryMapSize,
>+                    MemoryMap,
>+                    &MapKey,
>+                    &DescriptorSize,
>+                    &DescriptorVersion
>+                    );
>+    if (EFI_ERROR (Status)) {
>+      FreePool (MemoryMap);
>+    }
>+  } while (Status == EFI_BUFFER_TOO_SMALL);
>+  ASSERT_EFI_ERROR (Status);
>+
>+  DEBUG((DEBUG_ERROR, "%a: removing exec permissions from memory
>regions\n",
>+    __FUNCTION__));
>+
>+  MergeMemoryMapForProtectionPolicy (MemoryMap, &MemoryMapSize,
>DescriptorSize);
>+
>+  MemoryMapEntry = MemoryMap;
>+  MemoryMapEnd = (EFI_MEMORY_DESCRIPTOR *) ((UINT8 *) MemoryMap
>+ MemoryMapSize);
>+  while ((UINTN) MemoryMapEntry < (UINTN) MemoryMapEnd) {
>+
>+    Attributes = GetPermissionAttributeForMemoryType (MemoryMapEntry-
>>Type);
>+    if (Attributes != 0) {
>+      SetUefiImageMemoryAttributes (
>+        MemoryMapEntry->PhysicalStart,
>+        EFI_PAGES_TO_SIZE (MemoryMapEntry->NumberOfPages),
>+        Attributes);
>+    }
>+    MemoryMapEntry = NEXT_MEMORY_DESCRIPTOR (MemoryMapEntry,
>DescriptorSize);
>+  }
>+  FreePool (MemoryMap);
>+}
>+
>+
>+/**
>   A notification for CPU_ARCH protocol.
>
>   @param[in]  Event                 Event whose notification function is being
>invoked.
>@@ -674,6 +884,17 @@ MemoryProtectionCpuArchProtocolNotify (
>     return;
>   }
>
>+  //
>+  // Apply the memory protection policy on non-BScode/RTcode regions.
>+  //
>+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0) {
>+    InitializeDxeNxMemoryProtectionPolicy ();
>+  }
>+
>+  if (mImageProtectionPolicy == 0) {
>+    return;
>+  }
>+
>   Status = gBS->LocateHandleBuffer (
>                   ByProtocol,
>                   &gEfiLoadedImageProtocolGuid,
>@@ -753,7 +974,7 @@ CoreInitializeMemoryProtection (
>
>   mImageProtectionPolicy = PcdGet32(PcdImageProtectionPolicy);
>
>-  if (mImageProtectionPolicy != 0) {
>+  if (mImageProtectionPolicy != 0 || PcdGet64
>(PcdDxeNxMemoryProtectionPolicy) != 0) {
>     Status = CoreCreateEvent (
>                EVT_NOTIFY_SIGNAL,
>                TPL_CALLBACK,
>@@ -775,3 +996,86 @@ CoreInitializeMemoryProtection (
>   }
>   return ;
> }
>+
>+STATIC
>+BOOLEAN
>+IsInSmm (
>+  VOID
>+  )
>+{
>+  BOOLEAN     InSmm;
>+
>+  InSmm = FALSE;
>+  if (gSmmBase2 != NULL) {
>+    gSmmBase2->InSmm (gSmmBase2, &InSmm);
>+  }
>+  return InSmm;
>+}
>+
>+/**
>+  Manage memory permission attributes on a memory range, according to
>the
>+  configured DXE memory protection policy.
>+
>+  @param  OldType           The old memory type of the range
>+  @param  NewType           The new memory type of the range
>+  @param  Memory            The base address of the range
>+  @param  Length            The size of the range (in bytes)
>+
>+  @return EFI_SUCCESS       If we are executing in SMM mode. No permission
>attributes
>+                            are updated in this case
>+  @return EFI_SUCCESS       If the the CPU arch protocol is not installed yet
>+  @return EFI_SUCCESS       If no DXE memory protection policy has been
>configured
>+  @return EFI_SUCCESS       If OldType and NewType use the same permission
>attributes
>+  @return other             Return value of gCpu->SetMemoryAttributes()
>+
>+**/
>+EFI_STATUS
>+EFIAPI
>+ApplyMemoryProtectionPolicy (
>+  IN  EFI_MEMORY_TYPE       OldType,
>+  IN  EFI_MEMORY_TYPE       NewType,
>+  IN  EFI_PHYSICAL_ADDRESS  Memory,
>+  IN  UINT64                Length
>+  )
>+{
>+  UINT64  OldAttributes;
>+  UINT64  NewAttributes;
>+
>+  //
>+  // The policy configured in PcdDxeNxMemoryProtectionPolicy
>+  // does not apply to allocations performed in SMM mode.
>+  //
>+  if (IsInSmm ()) {
>+    return EFI_SUCCESS;
>+  }
>+
>+  //
>+  // If the CPU arch protocol is not installed yet, we cannot manage memory
>+  // permission attributes, and it is the job of the driver that installs this
>+  // protocol to set the permissions on existing allocations.
>+  //
>+  if (gCpu == NULL) {
>+    return EFI_SUCCESS;
>+  }
>+
>+  //
>+  // Check if a DXE memory protection policy has been configured
>+  //
>+  if (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) == 0) {
>+    return EFI_SUCCESS;
>+  }
>+
>+  //
>+  // Update the executable permissions according to the DXE memory
>+  // protection policy, but only if the policy is different between
>+  // the old and the new type.
>+  //
>+  OldAttributes = GetPermissionAttributeForMemoryType (OldType);
>+  NewAttributes = GetPermissionAttributeForMemoryType (NewType);
>+
>+  if (OldAttributes == NewAttributes) {
>+    return EFI_SUCCESS;
>+  }
>+
>+  return gCpu->SetMemoryAttributes (gCpu, Memory, Length,
>NewAttributes);
>+}
>--
>2.7.4

Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy

[Laszlo Ersek]

On 02/26/17 19:30, Ard Biesheuvel wrote:
> This implements a DXE memory protection policy that ensure that regions
> that don't require executable permissions are mapped with the non-exec
> attribute set.
> 
> First of all, it iterates over all entries in the UEFI memory map, and
> removes executable permissions according to the configured DXE memory
> protection policy, as recorded in PcdDxeMemoryProtectionPolicy.
> 
> Secondly, it sets or clears the non-executable attribute when allocating
> or freeing pages, both for page based or pool based allocations.
> 
> Note that this complements the image protection facility, which applies
> strict permissions to BootServicesCode/RuntimeServicesCode regions when
> the section alignment allows it. The memory protection configured by this
> patch operates on non-code regions only.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  MdeModulePkg/Core/Dxe/DxeMain.h               |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c              |   4 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c              |   7 +
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
>  5 files changed, 341 insertions(+), 1 deletion(-)

[snip]

> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 371e91cb0d7e..30d5984f7c1f 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -191,6 +191,7 @@ [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfileDriverPath                 ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
>  
>  # [Hob]
>  # RESOURCE_DESCRIPTOR   ## CONSUMES

The series doesn't build for  me:

.../MdeModulePkg/Core/Dxe/DxeMain.inf(194): error 3000: PCD
[gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy] in
[.../MdeModulePkg/Core/Dxe/DxeMain.inf] is not found in dependent packages:
                .../MdePkg/MdePkg.dec
        .../MdeModulePkg/MdeModulePkg.dec

I think you forgot to add the .dec hunk to this patch.

Thanks
Laszlo

Re: [PATCH v3 6/6] MdeModulePkg/DxeCore: implement memory protection policy

[Ard Biesheuvel]

On 27 February 2017 at 09:56, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/26/17 19:30, Ard Biesheuvel wrote:
>> This implements a DXE memory protection policy that ensure that regions
>> that don't require executable permissions are mapped with the non-exec
>> attribute set.
>>
>> First of all, it iterates over all entries in the UEFI memory map, and
>> removes executable permissions according to the configured DXE memory
>> protection policy, as recorded in PcdDxeMemoryProtectionPolicy.
>>
>> Secondly, it sets or clears the non-executable attribute when allocating
>> or freeing pages, both for page based or pool based allocations.
>>
>> Note that this complements the image protection facility, which applies
>> strict permissions to BootServicesCode/RuntimeServicesCode regions when
>> the section alignment allows it. The memory protection configured by this
>> patch operates on non-code regions only.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  MdeModulePkg/Core/Dxe/DxeMain.h               |  24 ++
>>  MdeModulePkg/Core/Dxe/DxeMain.inf             |   1 +
>>  MdeModulePkg/Core/Dxe/Mem/Page.c              |   4 +
>>  MdeModulePkg/Core/Dxe/Mem/Pool.c              |   7 +
>>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c | 306 +++++++++++++++++++-
>>  5 files changed, 341 insertions(+), 1 deletion(-)
>
> [snip]
>
>> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> index 371e91cb0d7e..30d5984f7c1f 100644
>> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
>> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
>> @@ -191,6 +191,7 @@ [Pcd]
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdMemoryProfileDriverPath                 ## CONSUMES
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable                   ## CONSUMES
>>    gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy                   ## CONSUMES
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy             ## CONSUMES
>>
>>  # [Hob]
>>  # RESOURCE_DESCRIPTOR   ## CONSUMES
>
> The series doesn't build for  me:
>
> .../MdeModulePkg/Core/Dxe/DxeMain.inf(194): error 3000: PCD
> [gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy] in
> [.../MdeModulePkg/Core/Dxe/DxeMain.inf] is not found in dependent packages:
>                 .../MdePkg/MdePkg.dec
>         .../MdeModulePkg/MdeModulePkg.dec
>
> I think you forgot to add the .dec hunk to this patch.
>

Apologies. I got the name wrong in the .dec, but I did update the
branch I pushed to the link above with the only change being a fix for
this.

Re: [PATCH v3 0/6] RFC: increased memory protection

[Yao, Jiewen]

Thanks Ard.

I found V3 5/6 has typo below:
+  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeMemoryProtectionPolicy|0x0000000|UINT64|0x00001048
It should be PcdDxeNxMemoryProtectionPolicy. Or I got build failure.

With above typo update, all series reviewed-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>

Regression Tested-by: Jiewen.yao@intel.com<mailto:Jiewen.yao@intel.com>
1) Default build (NX protection disable), boot Intel X86 system (X64 build) to UEFI Windows 10.
2) Default build (NX protection disable), boot Intel X86 system (IA32 build) to UEFI Shell.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Ard Biesheuvel [mailto:ard.biesheuvel@linaro.org]
> Sent: Monday, February 27, 2017 2:30 AM
> To: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>;
> leif.lindholm@linaro.org
> Cc: afish@apple.com; Kinney, Michael D <michael.d.kinney@intel.com>; Gao,
> Liming <liming.gao@intel.com>; lersek@redhat.com; Tian, Feng
> <feng.tian@intel.com>; Zeng, Star <star.zeng@intel.com>; Ard Biesheuvel
> <ard.biesheuvel@linaro.org>
> Subject: [PATCH v3 0/6] RFC: increased memory protection
>
> Hello all,
>
> This is a proof of concept implementation that removes all executable
> permissions from writable memory regions, which greatly enhances security.
> It is based on Jiewen's recent work, which is a step in the right direction,
> but still leaves most of memory exploitable due to the default R+W+X
> permissions.
>
> The idea is that the implementation of the CPU arch protocol goes over the
> memory map and removes exec permissions from all regions that are not already
> marked as 'code. This requires some preparatory work to ensure that the
> DxeCore
> itself is covered by a BootServicesCode region, not a BootServicesData region.
> Exec permissions are re-granted selectively, when the PE/COFF loader allocates
> the space for it. Combined with Jiewen's code/data split, this removes all
> RWX mapped regions.
>
> Changes since v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
>   prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
>
> Changes since v1:
> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have
>   the expected memory type (as suggested by Jiewen)
> - add patch to inhibit page table updates while syncing the GCD memory space
>   map with the page tables
> - add PCD to set memory protection policy, which allows the policy for reserved
>   and ACPI/NVS memory to be configured separately
> - move attribute manipulation into DxeCore page allocation code: this way, we
>   should be able to solve the EBC case by allocating BootServicesCode pool
>   memory explicitly.
>
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-tak
> e2
>
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
>
> Ard Biesheuvel (6):
>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>     images
>   MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
>   MdeModulePkg/DxeCore: use separate lock for pool allocations
>   MdeModulePkg: define PCD for DXE memory protection policy
>   MdeModulePkg/DxeCore: implement memory protection policy
>
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                     |   3 +
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                     |   1 +
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c               |   4 +
>  MdeModulePkg/Core/Dxe/DxeMain.h                    |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306
> +++++++++++++++++++-
>  MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
>  MdeModulePkg/MdeModulePkg.dec                      |  31 ++
>  MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
>  MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
>  MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
>  MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
>  MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
>  MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
>  16 files changed, 471 insertions(+), 18 deletions(-)
>
> --
> 2.7.4

Re: [PATCH v3 0/6] RFC: increased memory protection

[Laszlo Ersek]

On 02/26/17 19:29, Ard Biesheuvel wrote:
> Hello all,
> 
> This is a proof of concept implementation that removes all executable
> permissions from writable memory regions, which greatly enhances security.
> It is based on Jiewen's recent work, which is a step in the right direction,
> but still leaves most of memory exploitable due to the default R+W+X
> permissions.
> 
> The idea is that the implementation of the CPU arch protocol goes over the
> memory map and removes exec permissions from all regions that are not already
> marked as 'code. This requires some preparatory work to ensure that the DxeCore
> itself is covered by a BootServicesCode region, not a BootServicesData region.
> Exec permissions are re-granted selectively, when the PE/COFF loader allocates
> the space for it. Combined with Jiewen's code/data split, this removes all
> RWX mapped regions.
> 
> Changes since v2:
> - added patch to make EBC use EfiBootServicesCode pool allocations for thunks
> - redefine PCD according to Jiewen's feedback, including default value
> - use sorted memory map and merge adjacent entries with the same policy, to
>   prevent unnecessary page table splitting
> - ignore policy when executing in SMM
> - refactor the logic for managing permission attributes of pool allocations
> - added some R-b's
> 
> Changes since v1:
> - allocate code pages for PE/COFF images in PeiCore, so that DxeCore pages have
>   the expected memory type (as suggested by Jiewen)
> - add patch to inhibit page table updates while syncing the GCD memory space
>   map with the page tables
> - add PCD to set memory protection policy, which allows the policy for reserved
>   and ACPI/NVS memory to be configured separately
> - move attribute manipulation into DxeCore page allocation code: this way, we
>   should be able to solve the EBC case by allocating BootServicesCode pool
>   memory explicitly.
> 
> Series can be found here:
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=memprot-take2
> 
> Note that to test this properly, the default value of 0 should be changed
> to 0x7FD5, which applies non-exec permissions to everything except Efi*Code
> regions.
> 
> Ard Biesheuvel (6):
>   ArmPkg/CpuDxe: ignore attribute changes during SyncCacheConfig()
>   MdeModulePkg/PeiCore: allocate BootServicesCode memory for PE/COFF
>     images
>   MdeModulePkg/EbcDxe: use EfiBootServicesCode memory for thunks
>   MdeModulePkg/DxeCore: use separate lock for pool allocations
>   MdeModulePkg: define PCD for DXE memory protection policy
>   MdeModulePkg/DxeCore: implement memory protection policy
> 
>  ArmPkg/Drivers/CpuDxe/CpuDxe.c                     |   3 +
>  ArmPkg/Drivers/CpuDxe/CpuDxe.h                     |   1 +
>  ArmPkg/Drivers/CpuDxe/CpuMmuCommon.c               |   4 +
>  MdeModulePkg/Core/Dxe/DxeMain.h                    |  24 ++
>  MdeModulePkg/Core/Dxe/DxeMain.inf                  |   1 +
>  MdeModulePkg/Core/Dxe/Mem/Page.c                   |   4 +
>  MdeModulePkg/Core/Dxe/Mem/Pool.c                   |  60 +++-
>  MdeModulePkg/Core/Dxe/Misc/MemoryProtection.c      | 306 +++++++++++++++++++-
>  MdeModulePkg/Core/Pei/Image/Image.c                |  10 +-
>  MdeModulePkg/MdeModulePkg.dec                      |  31 ++
>  MdeModulePkg/Universal/EbcDxe/AArch64/EbcSupport.c |   2 +-
>  MdeModulePkg/Universal/EbcDxe/EbcInt.c             |  23 ++
>  MdeModulePkg/Universal/EbcDxe/EbcInt.h             |  14 +
>  MdeModulePkg/Universal/EbcDxe/Ia32/EbcSupport.c    |   2 +-
>  MdeModulePkg/Universal/EbcDxe/Ipf/EbcSupport.c     |   2 +-
>  MdeModulePkg/Universal/EbcDxe/X64/EbcSupport.c     |   2 +-
>  16 files changed, 471 insertions(+), 18 deletions(-)
> 

with the default 0 value for the PCD:

Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

For testing I more or less used
<https://github.com/tianocore/tianocore.github.io/wiki/Testing-SMM-with-QEMU,-KVM-and-libvirt>,
plus booted a few guests on aarch64/KVM with this (Fedora 24, RHEL-7.3,
openSUSE Tumbleweed).

Thanks
Laszlo