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

["Gao, Liming" <liming.gao@intel.com>]

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