[PATCH 0/6] Check untested memory and EFI_MEMORY_RO

[PATCH 0/6] Check untested memory and EFI_MEMORY_RO

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

This patch series adds check for untested memory in GCD
and check EFI_RUNTIME_RO in UEFI mem attrib table.

The final result is:
1) untested memory is not present in SMM page table.
2) the PE code section of runtime service is not present
in SMM page table.

Jiewen Yao (6):
  MdePkg/SmmMemLib: Check for untested memory in GCD
  UefiCpuPkg/PiSmmCpu: Check for untested memory in GCD
  MdeModulePkg/DxeCore: Install UEFI mem attrib table at EndOfDxe.
  MdePkg/SmmMemLib: Check EFI_MEMORY_RO in UEFI mem attrib table.
  UefiCpuPkg/PiSmmCpu: Check EFI_RUNTIME_RO in UEFI mem attrib table.
  MdeModulePkg/DxeCore: Not update RtCode in MemAttrTable after EndOfDxe

 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  36 +++-
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c       |  13 ++
 MdePkg/Library/SmmMemLib/SmmMemLib.c               | 152 +++++++++++++-
 MdePkg/Library/SmmMemLib/SmmMemLib.inf             |   5 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |   2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 217 +++++++++++++++++---
 7 files changed, 392 insertions(+), 34 deletions(-)

-- 
2.16.2.windows.1

[PATCH 1/6] MdePkg/SmmMemLib: Check for untested memory in GCD

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

It treats GCD untested memory as invalid SMM
communication buffer.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdePkg/Library/SmmMemLib/SmmMemLib.c   | 96 +++++++++++++++++++-
 MdePkg/Library/SmmMemLib/SmmMemLib.inf |  1 +
 2 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/MdePkg/Library/SmmMemLib/SmmMemLib.c b/MdePkg/Library/SmmMemLib/SmmMemLib.c
index 8c78a0b426..3f79e46d46 100644
--- a/MdePkg/Library/SmmMemLib/SmmMemLib.c
+++ b/MdePkg/Library/SmmMemLib/SmmMemLib.c
@@ -25,12 +25,20 @@
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Library/DxeServicesTableLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/HobLib.h>
 #include <Protocol/SmmAccess2.h>
 #include <Protocol/SmmReadyToLock.h>
 #include <Protocol/SmmEndOfDxe.h>
 
+//
+// attributes for reserved memory before it is promoted to system memory
+//
+#define EFI_MEMORY_PRESENT      0x0100000000000000ULL
+#define EFI_MEMORY_INITIALIZED  0x0200000000000000ULL
+#define EFI_MEMORY_TESTED       0x0400000000000000ULL
+
 #define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
   ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size)))
 
@@ -46,10 +54,13 @@ UINTN                 mMemoryMapEntryCount;
 EFI_MEMORY_DESCRIPTOR *mMemoryMap;
 UINTN                 mDescriptorSize;
 
+EFI_GCD_MEMORY_SPACE_DESCRIPTOR   *mSmmMemLibGcdMemSpace       = NULL;
+UINTN                             mSmmMemLibGcdMemNumberOfDesc = 0;
+
 VOID                  *mRegistrationEndOfDxe;
 VOID                  *mRegistrationReadyToLock;
 
-BOOLEAN               mSmmReadyToLock = FALSE;
+BOOLEAN               mSmmMemLibSmmReadyToLock = FALSE;
 
 /**
   Calculate and save the maximum support address.
@@ -154,7 +165,7 @@ SmmIsBufferOutsideSmmValid (
   //
   // Check override for Valid Communication Region
   //
-  if (mSmmReadyToLock) {
+  if (mSmmMemLibSmmReadyToLock) {
     EFI_MEMORY_DESCRIPTOR          *MemoryMap;
     BOOLEAN                        InValidCommunicationRegion;
 
@@ -171,12 +182,28 @@ SmmIsBufferOutsideSmmValid (
     if (!InValidCommunicationRegion) {
       DEBUG ((
         EFI_D_ERROR,
-        "SmmIsBufferOutsideSmmValid: Not in ValidCommunicationRegion: Buffer (0x%lx) - Length (0x%lx), ",
+        "SmmIsBufferOutsideSmmValid: Not in ValidCommunicationRegion: Buffer (0x%lx) - Length (0x%lx)\n",
         Buffer,
         Length
         ));
       return FALSE;
     }
+
+    //
+    // Check untested memory as invalid communication buffer.
+    //
+    for (Index = 0; Index < mSmmMemLibGcdMemNumberOfDesc; Index++) {
+      if (((Buffer >= mSmmMemLibGcdMemSpace[Index].BaseAddress) && (Buffer < mSmmMemLibGcdMemSpace[Index].BaseAddress + mSmmMemLibGcdMemSpace[Index].Length)) ||
+          ((mSmmMemLibGcdMemSpace[Index].BaseAddress >= Buffer) && (mSmmMemLibGcdMemSpace[Index].BaseAddress < Buffer + Length))) {
+        DEBUG ((
+          EFI_D_ERROR,
+          "SmmIsBufferOutsideSmmValid: In Untested Memory Region: Buffer (0x%lx) - Length (0x%lx)\n",
+          Buffer,
+          Length
+          ));
+        return FALSE;
+      }
+    }
   }
   return TRUE;
 }
@@ -317,6 +344,61 @@ SmmSetMem (
   return EFI_SUCCESS;
 }
 
+/**
+  Get GCD memory map.
+  Only record untested memory as invalid communication buffer.
+**/
+VOID
+SmmMemLibInternalGetGcdMemoryMap (
+  VOID
+  )
+{
+  UINTN                            NumberOfDescriptors;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *MemSpaceMap;
+  EFI_STATUS                       Status;
+  UINTN                            Index;
+
+  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemSpaceMap);
+  if (EFI_ERROR (Status)) {
+    return ;
+  }
+
+  mSmmMemLibGcdMemNumberOfDesc = 0;
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    if (MemSpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeReserved &&
+        (MemSpaceMap[Index].Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+          (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)
+          ) {
+      mSmmMemLibGcdMemNumberOfDesc++;
+    }
+  }
+
+  mSmmMemLibGcdMemSpace = AllocateZeroPool (mSmmMemLibGcdMemNumberOfDesc * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+  ASSERT (mSmmMemLibGcdMemSpace != NULL);
+  if (mSmmMemLibGcdMemSpace == NULL) {
+    mSmmMemLibGcdMemNumberOfDesc = 0;
+    gBS->FreePool (MemSpaceMap);
+    return ;
+  }
+
+  mSmmMemLibGcdMemNumberOfDesc = 0;
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    if (MemSpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeReserved &&
+        (MemSpaceMap[Index].Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+          (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)
+          ) {
+      CopyMem (
+        &mSmmMemLibGcdMemSpace[mSmmMemLibGcdMemNumberOfDesc],
+        &MemSpaceMap[Index],
+        sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR)
+        );
+      mSmmMemLibGcdMemNumberOfDesc++;
+    }
+  }
+
+  gBS->FreePool (MemSpaceMap);
+}
+
 /**
   Notification for SMM EndOfDxe protocol.
 
@@ -415,10 +497,14 @@ SmmLibInternalEndOfDxeNotify (
 
   gBS->FreePool (MemoryMap);
 
+  //
+  // Get additional information from GCD memory map.
+  //
+  SmmMemLibInternalGetGcdMemoryMap ();
+
   return EFI_SUCCESS;
 }
 
-
 /**
   Notification for SMM ReadyToLock protocol.
 
@@ -436,7 +522,7 @@ SmmLibInternalReadyToLockNotify (
   IN EFI_HANDLE      Handle
   )
 {
-  mSmmReadyToLock = TRUE;
+  mSmmMemLibSmmReadyToLock = TRUE;
   return EFI_SUCCESS;
 }
 /**
diff --git a/MdePkg/Library/SmmMemLib/SmmMemLib.inf b/MdePkg/Library/SmmMemLib/SmmMemLib.inf
index e4edad3af2..36576a4f2f 100644
--- a/MdePkg/Library/SmmMemLib/SmmMemLib.inf
+++ b/MdePkg/Library/SmmMemLib/SmmMemLib.inf
@@ -43,6 +43,7 @@
 [LibraryClasses]
   SmmServicesTableLib
   UefiBootServicesTableLib
+  DxeServicesTableLib
   DebugLib
   BaseMemoryLib
   HobLib
-- 
2.16.2.windows.1

[PATCH 2/6] UefiCpuPkg/PiSmmCpu: Check for untested memory in GCD

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

It treats GCD untested memory as invalid SMM
communication buffer.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 144 ++++++++++++++++----
 1 file changed, 120 insertions(+), 24 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 85e06b2e6a..b2ace6334e 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -13,6 +13,13 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 
 #include "PiSmmCpuDxeSmm.h"
 
+//
+// attributes for reserved memory before it is promoted to system memory
+//
+#define EFI_MEMORY_PRESENT      0x0100000000000000ULL
+#define EFI_MEMORY_INITIALIZED  0x0200000000000000ULL
+#define EFI_MEMORY_TESTED       0x0400000000000000ULL
+
 #define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
   ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size)))
 
@@ -23,6 +30,9 @@ EFI_MEMORY_DESCRIPTOR *mUefiMemoryMap;
 UINTN                 mUefiMemoryMapSize;
 UINTN                 mUefiDescriptorSize;
 
+EFI_GCD_MEMORY_SPACE_DESCRIPTOR   *mGcdMemSpace       = NULL;
+UINTN                             mGcdMemNumberOfDesc = 0;
+
 PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
   {Page4K,  SIZE_4KB, PAGING_4K_ADDRESS_MASK_64},
   {Page2M,  SIZE_2MB, PAGING_2M_ADDRESS_MASK_64},
@@ -1022,6 +1032,60 @@ MergeMemoryMapForNotPresentEntry (
   return ;
 }
 
+/**
+  This function caches the GCD memory map information.
+**/
+VOID
+GetGcdMemoryMap (
+  VOID
+  )
+{
+  UINTN                            NumberOfDescriptors;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *MemSpaceMap;
+  EFI_STATUS                       Status;
+  UINTN                            Index;
+
+  Status = gDS->GetMemorySpaceMap (&NumberOfDescriptors, &MemSpaceMap);
+  if (EFI_ERROR (Status)) {
+    return ;
+  }
+
+  mGcdMemNumberOfDesc = 0;
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    if (MemSpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeReserved &&
+        (MemSpaceMap[Index].Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+          (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)
+          ) {
+      mGcdMemNumberOfDesc++;
+    }
+  }
+
+  mGcdMemSpace = AllocateZeroPool (mGcdMemNumberOfDesc * sizeof (EFI_GCD_MEMORY_SPACE_DESCRIPTOR));
+  ASSERT (mGcdMemSpace != NULL);
+  if (mGcdMemSpace == NULL) {
+    mGcdMemNumberOfDesc = 0;
+    gBS->FreePool (MemSpaceMap);
+    return ;
+  }
+
+  mGcdMemNumberOfDesc = 0;
+  for (Index = 0; Index < NumberOfDescriptors; Index++) {
+    if (MemSpaceMap[Index].GcdMemoryType == EfiGcdMemoryTypeReserved &&
+        (MemSpaceMap[Index].Capabilities & (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED | EFI_MEMORY_TESTED)) ==
+          (EFI_MEMORY_PRESENT | EFI_MEMORY_INITIALIZED)
+          ) {
+      CopyMem (
+        &mGcdMemSpace[mGcdMemNumberOfDesc],
+        &MemSpaceMap[Index],
+        sizeof(EFI_GCD_MEMORY_SPACE_DESCRIPTOR)
+        );
+      mGcdMemNumberOfDesc++;
+    }
+  }
+
+  gBS->FreePool (MemSpaceMap);
+}
+
 /**
   This function caches the UEFI memory map information.
 **/
@@ -1081,6 +1145,11 @@ GetUefiMemoryMap (
   ASSERT (mUefiMemoryMap != NULL);
 
   gBS->FreePool (MemoryMap);
+
+  //
+  // Get additional information from GCD memory map.
+  //
+  GetGcdMemoryMap ();
 }
 
 /**
@@ -1102,33 +1171,52 @@ SetUefiMemMapAttributes (
 
   DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
 
-  if (mUefiMemoryMap == NULL) {
-    DEBUG ((DEBUG_INFO, "UefiMemoryMap - NULL\n"));
-    return ;
+  if (mUefiMemoryMap != NULL) {
+    MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
+    MemoryMap = mUefiMemoryMap;
+    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
+      if (IsUefiPageNotPresent(MemoryMap)) {
+        Status = SmmSetMemoryAttributes (
+                   MemoryMap->PhysicalStart,
+                   EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+                   EFI_MEMORY_RP
+                   );
+        DEBUG ((
+          DEBUG_INFO,
+          "UefiMemory protection: 0x%lx - 0x%lx %r\n",
+          MemoryMap->PhysicalStart,
+          MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+          Status
+          ));
+      }
+      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
+    }
   }
+  //
+  // Do not free mUefiMemoryMap, it will be checked in IsSmmCommBufferForbiddenAddress().
+  //
 
-  MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
-  MemoryMap = mUefiMemoryMap;
-  for (Index = 0; Index < MemoryMapEntryCount; Index++) {
-    if (IsUefiPageNotPresent(MemoryMap)) {
+  //
+  // Set untested memory as not present.
+  //
+  if (mGcdMemSpace != NULL) {
+    for (Index = 0; Index < mGcdMemNumberOfDesc; Index++) {
       Status = SmmSetMemoryAttributes (
-                 MemoryMap->PhysicalStart,
-                 EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+                 mGcdMemSpace[Index].BaseAddress,
+                 mGcdMemSpace[Index].Length,
                  EFI_MEMORY_RP
                  );
       DEBUG ((
         DEBUG_INFO,
-        "UefiMemory protection: 0x%lx - 0x%lx %r\n",
-        MemoryMap->PhysicalStart,
-        MemoryMap->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages),
+        "GcdMemory protection: 0x%lx - 0x%lx %r\n",
+        mGcdMemSpace[Index].BaseAddress,
+        mGcdMemSpace[Index].BaseAddress + mGcdMemSpace[Index].Length,
         Status
         ));
     }
-    MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
   }
-
   //
-  // Do free mUefiMemoryMap, it will be checked in IsSmmCommBufferForbiddenAddress().
+  // Do not free mGcdMemSpace, it will be checked in IsSmmCommBufferForbiddenAddress().
   //
 }
 
@@ -1149,21 +1237,29 @@ IsSmmCommBufferForbiddenAddress (
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
 
-  if (mUefiMemoryMap == NULL) {
-    return FALSE;
+  if (mUefiMemoryMap != NULL) {
+    MemoryMap = mUefiMemoryMap;
+    MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
+    for (Index = 0; Index < MemoryMapEntryCount; Index++) {
+      if (IsUefiPageNotPresent (MemoryMap)) {
+        if ((Address >= MemoryMap->PhysicalStart) &&
+            (Address < MemoryMap->PhysicalStart + EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)) ) {
+          return TRUE;
+        }
+      }
+      MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
+    }
   }
 
-  MemoryMap = mUefiMemoryMap;
-  MemoryMapEntryCount = mUefiMemoryMapSize/mUefiDescriptorSize;
-  for (Index = 0; Index < MemoryMapEntryCount; Index++) {
-    if (IsUefiPageNotPresent (MemoryMap)) {
-      if ((Address >= MemoryMap->PhysicalStart) &&
-          (Address < MemoryMap->PhysicalStart + EFI_PAGES_TO_SIZE((UINTN)MemoryMap->NumberOfPages)) ) {
+  if (mGcdMemSpace != NULL) {
+    for (Index = 0; Index < mGcdMemNumberOfDesc; Index++) {
+      if ((Address >= mGcdMemSpace[Index].BaseAddress) &&
+          (Address < mGcdMemSpace[Index].BaseAddress + mGcdMemSpace[Index].Length) ) {
         return TRUE;
       }
     }
-    MemoryMap = NEXT_MEMORY_DESCRIPTOR(MemoryMap, mUefiDescriptorSize);
   }
+
   return FALSE;
 }
 
-- 
2.16.2.windows.1

[PATCH 3/6] MdeModulePkg/DxeCore: Install UEFI mem attrib table at EndOfDxe.

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

So that the SMM can consume it to set page protection for
the UEFI runtime page with EFI_MEMORY_RO attribute.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c | 36 +++++++++++++++++++-
 1 file changed, 35 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
index 43e5be8b54..60557faa1c 100644
--- a/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c
@@ -240,6 +240,23 @@ InstallMemoryAttributesTableOnReadyToBoot (
   mMemoryAttributesTableReadyToBoot = TRUE;
 }
 
+/**
+  Install initial MemoryAttributesTable on EndOfDxe.
+  Then SMM can consume this information.
+
+  @param[in] Event      The Event this notify function registered to.
+  @param[in] Context    Pointer to the context data registered to the Event.
+**/
+VOID
+EFIAPI
+InstallMemoryAttributesTableOnEndOfDxe (
+  IN EFI_EVENT          Event,
+  IN VOID               *Context
+  )
+{
+  InstallMemoryAttributesTable ();
+}
+
 /**
   Initialize MemoryAttrubutesTable support.
 **/
@@ -251,18 +268,35 @@ CoreInitializeMemoryAttributesTable (
 {
   EFI_STATUS  Status;
   EFI_EVENT   ReadyToBootEvent;
+  EFI_EVENT   EndOfDxeEvent;
 
   //
   // Construct the table at ReadyToBoot.
   //
   Status = CoreCreateEventInternal (
              EVT_NOTIFY_SIGNAL,
-             TPL_CALLBACK - 1,
+             TPL_CALLBACK,
              InstallMemoryAttributesTableOnReadyToBoot,
              NULL,
              &gEfiEventReadyToBootGuid,
              &ReadyToBootEvent
              );
   ASSERT_EFI_ERROR (Status);
+
+  //
+  // Construct the initial table at EndOfDxe,
+  // then SMM can consume this information.
+  // Use TPL_NOTIFY here, as such SMM code (TPL_CALLBACK)
+  // can run after it.
+  //
+  Status = CoreCreateEventInternal (
+             EVT_NOTIFY_SIGNAL,
+             TPL_NOTIFY,
+             InstallMemoryAttributesTableOnEndOfDxe,
+             NULL,
+             &gEfiEndOfDxeEventGroupGuid,
+             &EndOfDxeEvent
+             );
+  ASSERT_EFI_ERROR (Status);
   return ;
 }
-- 
2.16.2.windows.1

[PATCH 4/6] MdePkg/SmmMemLib: Check EFI_MEMORY_RO in UEFI mem attrib table.

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

It treats the UEFI runtime page with EFI_MEMORY_RO attribute as
invalid SMM communication buffer.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdePkg/Library/SmmMemLib/SmmMemLib.c   | 58 +++++++++++++++++++-
 MdePkg/Library/SmmMemLib/SmmMemLib.inf |  4 ++
 2 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/MdePkg/Library/SmmMemLib/SmmMemLib.c b/MdePkg/Library/SmmMemLib/SmmMemLib.c
index 3f79e46d46..3409ddf87c 100644
--- a/MdePkg/Library/SmmMemLib/SmmMemLib.c
+++ b/MdePkg/Library/SmmMemLib/SmmMemLib.c
@@ -27,10 +27,12 @@
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/DxeServicesTableLib.h>
 #include <Library/SmmServicesTableLib.h>
+#include <Library/UefiLib.h>
 #include <Library/HobLib.h>
 #include <Protocol/SmmAccess2.h>
 #include <Protocol/SmmReadyToLock.h>
 #include <Protocol/SmmEndOfDxe.h>
+#include <Guid/MemoryAttributesTable.h>
 
 //
 // attributes for reserved memory before it is promoted to system memory
@@ -39,9 +41,6 @@
 #define EFI_MEMORY_INITIALIZED  0x0200000000000000ULL
 #define EFI_MEMORY_TESTED       0x0400000000000000ULL
 
-#define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
-  ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size)))
-
 EFI_SMRAM_DESCRIPTOR *mSmmMemLibInternalSmramRanges;
 UINTN                mSmmMemLibInternalSmramCount;
 
@@ -57,6 +56,8 @@ UINTN                 mDescriptorSize;
 EFI_GCD_MEMORY_SPACE_DESCRIPTOR   *mSmmMemLibGcdMemSpace       = NULL;
 UINTN                             mSmmMemLibGcdMemNumberOfDesc = 0;
 
+EFI_MEMORY_ATTRIBUTES_TABLE  *mSmmMemLibMemoryAttributesTable = NULL;
+
 VOID                  *mRegistrationEndOfDxe;
 VOID                  *mRegistrationReadyToLock;
 
@@ -204,6 +205,32 @@ SmmIsBufferOutsideSmmValid (
         return FALSE;
       }
     }
+
+    //
+    // Check UEFI runtime memory with EFI_MEMORY_RO as invalid communication buffer.
+    //
+    if (mSmmMemLibMemoryAttributesTable != NULL) {
+      EFI_MEMORY_DESCRIPTOR *Entry;
+
+      Entry = (EFI_MEMORY_DESCRIPTOR *)(mSmmMemLibMemoryAttributesTable + 1);
+      for (Index = 0; Index < mSmmMemLibMemoryAttributesTable->NumberOfEntries; Index++) {
+        if (Entry->Type == EfiRuntimeServicesCode || Entry->Type == EfiRuntimeServicesData) {
+          if ((Entry->Attribute & EFI_MEMORY_RO) != 0) {
+            if (((Buffer >= Entry->PhysicalStart) && (Buffer < Entry->PhysicalStart + LShiftU64 (Entry->NumberOfPages, EFI_PAGE_SHIFT))) ||
+                ((Entry->PhysicalStart >= Buffer) && (Entry->PhysicalStart < Buffer + Length))) {
+              DEBUG ((
+                EFI_D_ERROR,
+                "SmmIsBufferOutsideSmmValid: In RuntimeCode Region: Buffer (0x%lx) - Length (0x%lx)\n",
+                Buffer,
+                Length
+                ));
+              return FALSE;
+            }
+          }
+        }
+        Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mSmmMemLibMemoryAttributesTable->DescriptorSize);
+      }
+    }
   }
   return TRUE;
 }
@@ -399,6 +426,26 @@ SmmMemLibInternalGetGcdMemoryMap (
   gBS->FreePool (MemSpaceMap);
 }
 
+/**
+  Get UEFI MemoryAttributesTable.
+**/
+VOID
+SmmMemLibInternalGetUefiMemoryAttributesTable (
+  VOID
+  )
+{
+  EFI_STATUS                   Status;
+  EFI_MEMORY_ATTRIBUTES_TABLE  *MemoryAttributesTable;
+  UINTN                        MemoryAttributesTableSize;
+
+  Status = EfiGetSystemConfigurationTable (&gEfiMemoryAttributesTableGuid, (VOID **)&MemoryAttributesTable);
+  if (!EFI_ERROR (Status)) {
+    MemoryAttributesTableSize = sizeof(EFI_MEMORY_ATTRIBUTES_TABLE) + MemoryAttributesTable->DescriptorSize * MemoryAttributesTable->NumberOfEntries;
+    mSmmMemLibMemoryAttributesTable = AllocateCopyPool (MemoryAttributesTableSize, MemoryAttributesTable);
+    ASSERT (mSmmMemLibMemoryAttributesTable != NULL);
+  }
+}
+
 /**
   Notification for SMM EndOfDxe protocol.
 
@@ -502,6 +549,11 @@ SmmLibInternalEndOfDxeNotify (
   //
   SmmMemLibInternalGetGcdMemoryMap ();
 
+  //
+  // Get UEFI memory attributes table.
+  //
+  SmmMemLibInternalGetUefiMemoryAttributesTable ();
+
   return EFI_SUCCESS;
 }
 
diff --git a/MdePkg/Library/SmmMemLib/SmmMemLib.inf b/MdePkg/Library/SmmMemLib/SmmMemLib.inf
index 36576a4f2f..525449c25c 100644
--- a/MdePkg/Library/SmmMemLib/SmmMemLib.inf
+++ b/MdePkg/Library/SmmMemLib/SmmMemLib.inf
@@ -48,11 +48,15 @@
   BaseMemoryLib
   HobLib
   MemoryAllocationLib
+  UefiLib
 
 [Protocols]
   gEfiSmmAccess2ProtocolGuid     ## CONSUMES
   gEfiSmmReadyToLockProtocolGuid ## CONSUMES
   gEfiSmmEndOfDxeProtocolGuid    ## CONSUMES
 
+[Guids]
+  gEfiMemoryAttributesTableGuid  ## CONSUMES ## SystemTable
+
 [Depex]
   gEfiSmmAccess2ProtocolGuid
-- 
2.16.2.windows.1

[PATCH 5/6] UefiCpuPkg/PiSmmCpu: Check EFI_RUNTIME_RO in UEFI mem attrib table.

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

It treats the UEFI runtime page with EFI_MEMORY_RO attribute as
invalid SMM communication buffer.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |  2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |  1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 75 +++++++++++++++++++-
 3 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 1d016594e0..e3c7cff81c 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -28,6 +28,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Protocol/SmmMemoryAttribute.h>
 
 #include <Guid/AcpiS3Context.h>
+#include <Guid/MemoryAttributesTable.h>
 #include <Guid/PiSmmMemoryAttributesTable.h>
 
 #include <Library/BaseLib.h>
@@ -45,6 +46,7 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/UefiRuntimeServicesTableLib.h>
 #include <Library/DebugAgentLib.h>
+#include <Library/UefiLib.h>
 #include <Library/HobLib.h>
 #include <Library/LocalApicLib.h>
 #include <Library/UefiCpuLib.h>
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
index 52d8c55075..a7fb7b0b14 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf
@@ -117,6 +117,7 @@
   gEfiAcpi20TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
   gEfiAcpi10TableGuid                      ## SOMETIMES_CONSUMES ## SystemTable
   gEdkiiPiSmmMemoryAttributesTableGuid     ## CONSUMES ## SystemTable
+  gEfiMemoryAttributesTableGuid            ## CONSUMES ## SystemTable
 
 [FeaturePcd]
   gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmDebug                         ## CONSUMES
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index b2ace6334e..7d99f23426 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -20,9 +20,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
 #define EFI_MEMORY_INITIALIZED  0x0200000000000000ULL
 #define EFI_MEMORY_TESTED       0x0400000000000000ULL
 
-#define NEXT_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
-  ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) + (Size)))
-
 #define PREVIOUS_MEMORY_DESCRIPTOR(MemoryDescriptor, Size) \
   ((EFI_MEMORY_DESCRIPTOR *)((UINT8 *)(MemoryDescriptor) - (Size)))
 
@@ -33,6 +30,8 @@ UINTN                 mUefiDescriptorSize;
 EFI_GCD_MEMORY_SPACE_DESCRIPTOR   *mGcdMemSpace       = NULL;
 UINTN                             mGcdMemNumberOfDesc = 0;
 
+EFI_MEMORY_ATTRIBUTES_TABLE  *mUefiMemoryAttributesTable = NULL;
+
 PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
   {Page4K,  SIZE_4KB, PAGING_4K_ADDRESS_MASK_64},
   {Page2M,  SIZE_2MB, PAGING_2M_ADDRESS_MASK_64},
@@ -1086,6 +1085,26 @@ GetGcdMemoryMap (
   gBS->FreePool (MemSpaceMap);
 }
 
+/**
+  Get UEFI MemoryAttributesTable.
+**/
+VOID
+GetUefiMemoryAttributesTable (
+  VOID
+  )
+{
+  EFI_STATUS                   Status;
+  EFI_MEMORY_ATTRIBUTES_TABLE  *MemoryAttributesTable;
+  UINTN                        MemoryAttributesTableSize;
+
+  Status = EfiGetSystemConfigurationTable (&gEfiMemoryAttributesTableGuid, (VOID **)&MemoryAttributesTable);
+  if (!EFI_ERROR (Status)) {
+    MemoryAttributesTableSize = sizeof(EFI_MEMORY_ATTRIBUTES_TABLE) + MemoryAttributesTable->DescriptorSize * MemoryAttributesTable->NumberOfEntries;
+    mUefiMemoryAttributesTable = AllocateCopyPool (MemoryAttributesTableSize, MemoryAttributesTable);
+    ASSERT (mUefiMemoryAttributesTable != NULL);
+  }
+}
+
 /**
   This function caches the UEFI memory map information.
 **/
@@ -1150,6 +1169,11 @@ GetUefiMemoryMap (
   // Get additional information from GCD memory map.
   //
   GetGcdMemoryMap ();
+
+  //
+  // Get UEFI memory attributes table.
+  //
+  GetUefiMemoryAttributesTable ();
 }
 
 /**
@@ -1168,6 +1192,7 @@ SetUefiMemMapAttributes (
   EFI_MEMORY_DESCRIPTOR *MemoryMap;
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
+  EFI_MEMORY_DESCRIPTOR *Entry;
 
   DEBUG ((DEBUG_INFO, "SetUefiMemMapAttributes\n"));
 
@@ -1218,6 +1243,35 @@ SetUefiMemMapAttributes (
   //
   // Do not free mGcdMemSpace, it will be checked in IsSmmCommBufferForbiddenAddress().
   //
+
+  //
+  // Set UEFI runtime memory with EFI_MEMORY_RO as not present.
+  //
+  if (mUefiMemoryAttributesTable != NULL) {
+    Entry = (EFI_MEMORY_DESCRIPTOR *)(mUefiMemoryAttributesTable + 1);
+    for (Index = 0; Index < mUefiMemoryAttributesTable->NumberOfEntries; Index++) {
+      if (Entry->Type == EfiRuntimeServicesCode || Entry->Type == EfiRuntimeServicesData) {
+        if ((Entry->Attribute & EFI_MEMORY_RO) != 0) {
+          Status = SmmSetMemoryAttributes (
+                     Entry->PhysicalStart,
+                     EFI_PAGES_TO_SIZE((UINTN)Entry->NumberOfPages),
+                     EFI_MEMORY_RP
+                     );
+          DEBUG ((
+            DEBUG_INFO,
+            "UefiMemoryAttribute protection: 0x%lx - 0x%lx %r\n",
+            Entry->PhysicalStart,
+            Entry->PhysicalStart + (UINT64)EFI_PAGES_TO_SIZE((UINTN)Entry->NumberOfPages),
+            Status
+            ));
+        }
+      }
+      Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
+    }
+  }
+  //
+  // Do not free mUefiMemoryAttributesTable, it will be checked in IsSmmCommBufferForbiddenAddress().
+  //
 }
 
 /**
@@ -1236,6 +1290,7 @@ IsSmmCommBufferForbiddenAddress (
   EFI_MEMORY_DESCRIPTOR *MemoryMap;
   UINTN                 MemoryMapEntryCount;
   UINTN                 Index;
+  EFI_MEMORY_DESCRIPTOR *Entry;
 
   if (mUefiMemoryMap != NULL) {
     MemoryMap = mUefiMemoryMap;
@@ -1260,6 +1315,20 @@ IsSmmCommBufferForbiddenAddress (
     }
   }
 
+  if (mUefiMemoryAttributesTable != NULL) {
+    Entry = (EFI_MEMORY_DESCRIPTOR *)(mUefiMemoryAttributesTable + 1);
+    for (Index = 0; Index < mUefiMemoryAttributesTable->NumberOfEntries; Index++) {
+      if (Entry->Type == EfiRuntimeServicesCode || Entry->Type == EfiRuntimeServicesData) {
+        if ((Entry->Attribute & EFI_MEMORY_RO) != 0) {
+          if ((Address >= Entry->PhysicalStart) &&
+              (Address < Entry->PhysicalStart + LShiftU64 (Entry->NumberOfPages, EFI_PAGE_SHIFT))) {
+            return TRUE;
+          }
+          Entry = NEXT_MEMORY_DESCRIPTOR (Entry, mUefiMemoryAttributesTable->DescriptorSize);
+        }
+      }
+    }
+  }
   return FALSE;
 }
 
-- 
2.16.2.windows.1

[PATCH 6/6] MdeModulePkg/DxeCore: Not update RtCode in MemAttrTable after EndOfDxe

[Hao Wu]

From: Jiewen Yao <jiewen.yao@intel.com>

We want to provide precise info in MemAttribTable
to both OS and SMM, and SMM only gets the info at EndOfDxe.
So we do not update RtCode entry in EndOfDxe.

The impact is that if 3rd part OPROM is runtime, it cannot be executed
at UEFI runtime phase.
Currently, we do not see compatibility issue, because the only runtime
OPROM we found before in UNDI, and UEFI OS will not use UNDI interface
in OS.

Cc: Star Zeng <star.zeng@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
---
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
index a84507df95..a96d442fbc 100644
--- a/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
+++ b/MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c
@@ -62,6 +62,8 @@ EFI_LOCK           mPropertiesTableLock = EFI_INITIALIZE_LOCK_VARIABLE (TPL_NOTI
 
 BOOLEAN            mPropertiesTableEnable;
 
+BOOLEAN            mPropertiesTableEndOfDxe = FALSE;
+
 //
 // Below functions are for MemoryMap
 //
@@ -1079,6 +1081,11 @@ InsertImageRecord (
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%x\n", RuntimeImage));
   DEBUG ((EFI_D_VERBOSE, "InsertImageRecord - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize));
 
+  if (mPropertiesTableEndOfDxe) {
+    DEBUG ((DEBUG_INFO, "Do not insert runtime image record after EndOfDxe\n"));
+    return ;
+  }
+
   ImageRecord = AllocatePool (sizeof(*ImageRecord));
   if (ImageRecord == NULL) {
     return ;
@@ -1296,6 +1303,11 @@ RemoveImageRecord (
   DEBUG ((EFI_D_VERBOSE, "RemoveImageRecord - 0x%x\n", RuntimeImage));
   DEBUG ((EFI_D_VERBOSE, "RemoveImageRecord - 0x%016lx - 0x%016lx\n", (EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize));
 
+  if (mPropertiesTableEndOfDxe) {
+    DEBUG ((DEBUG_INFO, "Do not remove runtime image record after EndOfDxe\n"));
+    return ;
+  }
+
   ImageRecord = FindImageRecord ((EFI_PHYSICAL_ADDRESS)(UINTN)RuntimeImage->ImageBase, RuntimeImage->ImageSize);
   if (ImageRecord == NULL) {
     DEBUG ((EFI_D_ERROR, "!!!!!!!! ImageRecord not found !!!!!!!!\n"));
@@ -1333,6 +1345,7 @@ InstallPropertiesTable (
   VOID                                    *Context
   )
 {
+  mPropertiesTableEndOfDxe = TRUE;
   if (PcdGetBool (PcdPropertiesTableEnable)) {
     EFI_STATUS  Status;
 
-- 
2.16.2.windows.1

Re: [PATCH 0/6] Check untested memory and EFI_MEMORY_RO

[Zeng, Star]

Reviewed-by: Star Zeng <star.zeng@intel.com> to this patch series.

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Hao Wu
Sent: Friday, July 20, 2018 1:26 PM
To: edk2-devel@lists.01.org
Cc: Yao, Jiewen <jiewen.yao@intel.com>
Subject: [edk2] [PATCH 0/6] Check untested memory and EFI_MEMORY_RO

From: Jiewen Yao <jiewen.yao@intel.com>

This patch series adds check for untested memory in GCD and check EFI_RUNTIME_RO in UEFI mem attrib table.

The final result is:
1) untested memory is not present in SMM page table.
2) the PE code section of runtime service is not present in SMM page table.

Jiewen Yao (6):
  MdePkg/SmmMemLib: Check for untested memory in GCD
  UefiCpuPkg/PiSmmCpu: Check for untested memory in GCD
  MdeModulePkg/DxeCore: Install UEFI mem attrib table at EndOfDxe.
  MdePkg/SmmMemLib: Check EFI_MEMORY_RO in UEFI mem attrib table.
  UefiCpuPkg/PiSmmCpu: Check EFI_RUNTIME_RO in UEFI mem attrib table.
  MdeModulePkg/DxeCore: Not update RtCode in MemAttrTable after EndOfDxe

 MdeModulePkg/Core/Dxe/Misc/MemoryAttributesTable.c |  36 +++-
 MdeModulePkg/Core/Dxe/Misc/PropertiesTable.c       |  13 ++
 MdePkg/Library/SmmMemLib/SmmMemLib.c               | 152 +++++++++++++-
 MdePkg/Library/SmmMemLib/SmmMemLib.inf             |   5 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h         |   2 +
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.inf       |   1 +
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 217 +++++++++++++++++---
 7 files changed, 392 insertions(+), 34 deletions(-)

--
2.16.2.windows.1

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