[PATCH 0/2] fix DXE memory free issue in SMM mode

[PATCH 0/2] fix DXE memory free issue in SMM mode

[Jian J Wang]

This patch series try to fix an issue caused by trying to free memory
allocated in DXE but freed in SMM mode. This happens only if Heap
Guard feature is enabled, which needs to update page table. The root
cause is that DXE and SMM don't share the same paging configuration
but CpuDxe driver still tries to access page table through current
processor registers (CR3) in SMM mode, during memory free opration in
DXE core. This will cause DXE core got the incorrect paging attributes
of memory to be freed, and then fail the free operation.

The solution is let CpuDxe store the paging configuration in a global
variable and use it to access DXE page table if in SMM mode.

Jian J Wang (2):
  UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode
  MdeModulePkg/Core: remove SMM check for Heap Guard feature detection

 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c |  10 ----
 UefiCpuPkg/CpuDxe/CpuDxe.c            |   2 +-
 UefiCpuPkg/CpuDxe/CpuDxe.inf          |   1 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c      | 108 +++++++++++++++++++++++-----------
 4 files changed, 75 insertions(+), 46 deletions(-)

-- 
2.16.2.windows.1

[PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Jian J Wang]

The SMM version of MemoryAllocationLib allows to free memory allocated
in DXE (before EndOfDxe). This is done by checking the memory range and
calling gBS services to do real operation if the memory to free is out
of SMRAM. This would cause problem if some memory related features, like
Heap Guard, have to update page table to change memory attributes.
Because page table in SMM mode is different from DXE mode, gBS memory
services cannot get the correct attributes of DXE memory from SMM page
table and then cause incorrect memory manipulations.

The solution in this patch is to store the DXE page table information
(e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe
driver. If CpuDxe detects it's in SMM mode, it will use this global
variable to access page table instead of current processor registers.

Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 UefiCpuPkg/CpuDxe/CpuDxe.c       |   2 +-
 UefiCpuPkg/CpuDxe/CpuDxe.inf     |   1 +
 UefiCpuPkg/CpuDxe/CpuPageTable.c | 108 ++++++++++++++++++++++++++-------------
 3 files changed, 75 insertions(+), 36 deletions(-)

diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 6ae2dcd1c7..1fd996fc3f 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -404,7 +404,7 @@ CpuSetMemoryAttributes (
   // to avoid unnecessary computing.
   //
   if (mIsFlushingGCD) {
-    DEBUG((DEBUG_INFO, "  Flushing GCD\n"));
+    DEBUG((DEBUG_GCD, "  Flushing GCD\n"));
     return EFI_SUCCESS;
   }
 
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
index 3c938cee53..8c8773af90 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
@@ -66,6 +66,7 @@
 [Protocols]
   gEfiCpuArchProtocolGuid                       ## PRODUCES
   gEfiMpServiceProtocolGuid                     ## PRODUCES
+  gEfiSmmBase2ProtocolGuid
 
 [Guids]
   gIdleLoopEventGuid                            ## CONSUMES           ## Event
diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
index e2595b4d89..bf420d3792 100644
--- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
+++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
@@ -23,6 +23,7 @@
 #include <Library/DebugLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Protocol/MpService.h>
+#include <Protocol/SmmBase2.h>
 
 #include "CpuDxe.h"
 #include "CpuPageTable.h"
@@ -87,7 +88,33 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
   {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
 };
 
-PAGE_TABLE_POOL   *mPageTablePool = NULL;
+PAGE_TABLE_POOL                   *mPageTablePool = NULL;
+PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
+EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
+
+BOOLEAN
+IsInSmm (
+  VOID
+  )
+{
+  EFI_STATUS              Status;
+  BOOLEAN                 InSmm;
+
+  InSmm = FALSE;
+  if (mSmmBase2 == NULL) {
+    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
+                                  (VOID **)&mSmmBase2);
+    if (EFI_ERROR (Status)) {
+      mSmmBase2 = NULL;
+    }
+  }
+
+  if (mSmmBase2 != NULL) {
+    mSmmBase2->InSmm (mSmmBase2, &InSmm);
+  }
+
+  return InSmm;
+}
 
 /**
   Return current paging context.
@@ -102,42 +129,45 @@ GetCurrentPagingContext (
   UINT32                         RegEax;
   UINT32                         RegEdx;
 
-  ZeroMem(PagingContext, sizeof(*PagingContext));
-  if (sizeof(UINTN) == sizeof(UINT64)) {
-    PagingContext->MachineType = IMAGE_FILE_MACHINE_X64;
-  } else {
-    PagingContext->MachineType = IMAGE_FILE_MACHINE_I386;
-  }
-  if ((AsmReadCr0 () & BIT31) != 0) {
-    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
-  } else {
-    PagingContext->ContextData.X64.PageTableBase = 0;
-  }
+  if (!IsInSmm ()) {
+    if (sizeof(UINTN) == sizeof(UINT64)) {
+      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
+    } else {
+      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
+    }
+    if ((AsmReadCr0 () & BIT31) != 0) {
+      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
+    } else {
+      mPagingContext.ContextData.X64.PageTableBase = 0;
+    }
 
-  if ((AsmReadCr4 () & BIT4) != 0) {
-    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
-  }
-  if ((AsmReadCr4 () & BIT5) != 0) {
-    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
-  }
-  if ((AsmReadCr0 () & BIT16) != 0) {
-    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
-  }
+    if ((AsmReadCr4 () & BIT4) != 0) {
+      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
+    }
+    if ((AsmReadCr4 () & BIT5) != 0) {
+      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
+    }
+    if ((AsmReadCr0 () & BIT16) != 0) {
+      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
+    }
 
-  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
-  if (RegEax > 0x80000000) {
-    AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
-    if ((RegEdx & BIT20) != 0) {
-      // XD supported
-      if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
-        // XD activated
-        PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
+    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
+    if (RegEax > 0x80000000) {
+      AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
+      if ((RegEdx & BIT20) != 0) {
+        // XD supported
+        if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
+          // XD activated
+          mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
+        }
+      }
+      if ((RegEdx & BIT26) != 0) {
+        mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
       }
-    }
-    if ((RegEdx & BIT26) != 0) {
-      PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
     }
   }
+
+  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
 }
 
 /**
@@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
   VOID
   )
 {
-  return ((AsmReadCr0 () & BIT16) != 0);
+  if (!IsInSmm ()) {
+    return ((AsmReadCr0 () & BIT16) != 0);
+  }
+  return FALSE;
 }
 
 /**
@@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
   VOID
   )
 {
-  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
+  if (!IsInSmm ()) {
+    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);
+  }
 }
 
 /**
@@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
   VOID
   )
 {
-  AsmWriteCr0 (AsmReadCr0() | BIT16);
+  if (!IsInSmm ()) {
+    AsmWriteCr0 (AsmReadCr0 () | BIT16);
+  }
 }
 
 /**
@@ -1054,6 +1091,7 @@ InitializePageTableLib (
 {
   PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
 
+  ZeroMem (&mPagingContext, sizeof(mPagingContext));
   GetCurrentPagingContext (&CurrentPagingContext);
 
   //
-- 
2.16.2.windows.1

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Laszlo Ersek]

Hi Jian,

On 06/11/18 09:08, Jian J Wang wrote:
> The SMM version of MemoryAllocationLib allows to free memory allocated
> in DXE (before EndOfDxe). This is done by checking the memory range and
> calling gBS services to do real operation if the memory to free is out
> of SMRAM. This would cause problem if some memory related features, like
> Heap Guard, have to update page table to change memory attributes.
> Because page table in SMM mode is different from DXE mode, gBS memory
> services cannot get the correct attributes of DXE memory from SMM page
> table and then cause incorrect memory manipulations.

(1) I think this description makes sense, but it does not highlight the
involvement of CpuDxe.

(1a) Please reference "MdePkg/Library/SmmMemoryAllocationLib" specifically.

(1b) The SmmMemoryAllocationLib instance implies that the call chain
starts with a DXE_SMM_DRIVER calling FreePool() or FreePages().

DXE_SMM_DRIVERs can only call boot services, and protocols located with
boot services, in their entry point functions.

CpuDxe is a DXE_DRIVER and it provides the CPU arch protocol (and the MP
services protocol). Thus, our call chain can only occur if:

- a DXE_SMM_DRIVER calls the FreePool() or FreePages() library API,

- in its entry point function,

- releasing memory that's not SMRAM,

- and the heap guard feature is enabled, so the FreePool() or
  FreePages() boot service ends up calling
  EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes().

Please include this information in the commit message.

(2) Can you remind me how the SMM page tables map non-SMRAM memory?

Because, I assume that, after a FreePages() libary API call, the freed
memory should not be accessible to either SMM code or normal DXE code.
This seems to imply that both sets of page tables should be modified.
What am I missing?

> 
> The solution in this patch is to store the DXE page table information
> (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe
> driver. If CpuDxe detects it's in SMM mode, it will use this global
> variable to access page table instead of current processor registers.
> 
> Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f

(3) I think this comes from Gerrit. Do you really need the "Change-Id"
tag in the commit message? It doesn't say anything to the upstream edk2
users.

If you really need it, I don't mind it though; just please let's not add
it due to oversight.

(4) Please make sure that you don't *push* the patch with Gerrit. Gerrit
does bad things to git metadata. Please see
<http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.ccr.corp.intel.com>.

> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c       |   2 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.inf     |   1 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 108 ++++++++++++++++++++++++++-------------
>  3 files changed, 75 insertions(+), 36 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 6ae2dcd1c7..1fd996fc3f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -404,7 +404,7 @@ CpuSetMemoryAttributes (
>    // to avoid unnecessary computing.
>    //
>    if (mIsFlushingGCD) {
> -    DEBUG((DEBUG_INFO, "  Flushing GCD\n"));
> +    DEBUG((DEBUG_GCD, "  Flushing GCD\n"));
>      return EFI_SUCCESS;
>    }

(5) I think it would be cleaner if we fixed the debug level for this
message in a separate patch. You are not touching "CpuDxe.c" for any
other reason.

>  
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 3c938cee53..8c8773af90 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -66,6 +66,7 @@
>  [Protocols]
>    gEfiCpuArchProtocolGuid                       ## PRODUCES
>    gEfiMpServiceProtocolGuid                     ## PRODUCES
> +  gEfiSmmBase2ProtocolGuid

(6) protocol usage note is missing (should be "## SOMETIMES_CONSUMES" I
think.)

>  
>  [Guids]
>    gIdleLoopEventGuid                            ## CONSUMES           ## Event
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index e2595b4d89..bf420d3792 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -23,6 +23,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Protocol/MpService.h>
> +#include <Protocol/SmmBase2.h>
>  
>  #include "CpuDxe.h"
>  #include "CpuPageTable.h"
> @@ -87,7 +88,33 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>    {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
>  };
>  
> -PAGE_TABLE_POOL   *mPageTablePool = NULL;
> +PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
> +EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
> +
> +BOOLEAN
> +IsInSmm (
> +  VOID
> +  )
> +{
> +  EFI_STATUS              Status;
> +  BOOLEAN                 InSmm;
> +
> +  InSmm = FALSE;
> +  if (mSmmBase2 == NULL) {
> +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
> +                                  (VOID **)&mSmmBase2);

(7) Indentation is incorrect.

> +    if (EFI_ERROR (Status)) {
> +      mSmmBase2 = NULL;
> +    }

(8) I think you can simply drop the EFI_ERROR (Status) branch;
"mSmmBase2" will simply remain NULL.

> +  }
> +
> +  if (mSmmBase2 != NULL) {
> +    mSmmBase2->InSmm (mSmmBase2, &InSmm);
> +  }
> +
> +  return InSmm;
> +}

(9) I don't understand how the InSmm() member function is supposed to
work. According to the PI spec (v1.6):

"""
EFI_MM_BASE_PROTOCOL.InMm()
[...]
Service to indicate whether the driver is currently executing in the MM
Driver Initialization phase.
[...]
InMmram
Pointer to a Boolean which, on return, indicates that the driver is
currently executing inside of MMRAM (TRUE) or outside of MMRAM (FALSE).
[...]
This service returns whether the caller is being executed in the MM
Driver Initialization phase. For MM Drivers, this will return TRUE in
InMmram while inside the driver’s entry point and otherwise FALSE. For
combination MM/DXE drivers, this will return FALSE in the DXE launch.
For the MM launch, it behaves as an MM Driver.
"""

First of all, to my knowledge, we don't have any combination MM/DXE
drivers in edk2 (with dual entry point launches, binary type 0x0C). We
have traditional MM drivers (binary type 0x0A) that are launched
directly from SMRAM. However, it is not guaranteed that they are
launched with the processor operating in System Management Mode.

Secondly, focusing on traditional MM drivers, the InMm() specification
doesn't seem to say anything as to whether the processor is running in
SMM (system management mode) or normal mode. Initially, SMRAM is open,
and thus the processor can execute MM driver code out of it without
actually operating in SMM (see above).

Furthermore, CpuDxe is a DXE_DRIVER, thus it is never loaded into SMRAM.

I notice that you call the "InMmram" parameter "InSmm". Are you saying
that the PI spec is wrong, and edk2 actually returns whether the
processor is executing CpuDxe code in Management Mode? (Due to a call
from the entry point function of a traditional MM driver?)

Third, the spec is inconsistent even with itself. The general
description says, "For MM Drivers, this will return TRUE in InMmram
while inside the driver’s entry point and otherwise FALSE". But a DXE
protocol must not even be invoked from a traditional MM driver *except*
from the entry point! Which would imply that, for traditional MM
drivers, the output parameter could only be set to TRUE.

So basically what I'd like to see here is evidence that

(9a) the IsInSmm() helper function is never called outside of the entry
     point of an MM driver, due to the restrictions on
     gBS->LocateProtocol() and mSmmBase2->InSmm(),

(9b) the mSmmBase2->InSmm() protocol member determines whether CpuDxe is
     being executed by the processor in Management Mode, and *not*
     whether the CpuDxe driver is being executed from within SMRAM.

>  
>  /**
>    Return current paging context.
> @@ -102,42 +129,45 @@ GetCurrentPagingContext (
>    UINT32                         RegEax;
>    UINT32                         RegEdx;
>  
> -  ZeroMem(PagingContext, sizeof(*PagingContext));
> -  if (sizeof(UINTN) == sizeof(UINT64)) {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_X64;
> -  } else {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_I386;
> -  }
> -  if ((AsmReadCr0 () & BIT31) != 0) {
> -    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> -  } else {
> -    PagingContext->ContextData.X64.PageTableBase = 0;
> -  }
> +  if (!IsInSmm ()) {
> +    if (sizeof(UINTN) == sizeof(UINT64)) {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> +    } else {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> +    }
> +    if ((AsmReadCr0 () & BIT31) != 0) {
> +      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> +    } else {
> +      mPagingContext.ContextData.X64.PageTableBase = 0;
> +    }
>  
> -  if ((AsmReadCr4 () & BIT4) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> -  }
> -  if ((AsmReadCr4 () & BIT5) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> -  }
> -  if ((AsmReadCr0 () & BIT16) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> -  }
> +    if ((AsmReadCr4 () & BIT4) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> +    }
> +    if ((AsmReadCr4 () & BIT5) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> +    }
> +    if ((AsmReadCr0 () & BIT16) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> +    }
>  
> -  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> -  if (RegEax > 0x80000000) {
> -    AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> -    if ((RegEdx & BIT20) != 0) {
> -      // XD supported
> -      if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> -        // XD activated
> -        PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> +    if (RegEax > 0x80000000) {
> +      AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> +      if ((RegEdx & BIT20) != 0) {
> +        // XD supported
> +        if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> +          // XD activated
> +          mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +        }
> +      }
> +      if ((RegEdx & BIT26) != 0) {
> +        mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>        }
> -    }
> -    if ((RegEdx & BIT26) != 0) {
> -      PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>      }
>    }
> +
> +  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
>  }

(10) Even assuming that IsInSmm() always returns false -- for example
because the platform does not include the SMM drivers --, this change
does not look like a no-op to me.

The ZeroMem() call at the top is removed by the patch, but attributes
like PSE, PAE, WP_ENABLE, XD_ACTIVATED and 1G_SUPPORT are never
*cleared* in "mPagingContext", once they are set.

I guess the idea is that a few (how many?) initial calls to
GetCurrentPagingContext() set up "mPagingContext", and after that,
GetCurrentPagingContext() just keeps returning the last set context. But
I don't think that those "initial calls" work correctly.

>  
>  /**
> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
>    VOID
>    )
>  {
> -  return ((AsmReadCr0 () & BIT16) != 0);
> +  if (!IsInSmm ()) {
> +    return ((AsmReadCr0 () & BIT16) != 0);
> +  }
> +  return FALSE;
>  }
>  
>  /**
> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);
> +  }
>  }
>  
>  /**
> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () | BIT16);
> +  }
>  }

(11) I don't understand these changes at all. Why are these functions
no-ops when CpuDxe is invoked in Management Mode?

Thanks
Laszlo

>  
>  /**
> @@ -1054,6 +1091,7 @@ InitializePageTableLib (
>  {
>    PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
>  
> +  ZeroMem (&mPagingContext, sizeof(mPagingContext));
>    GetCurrentPagingContext (&CurrentPagingContext);
>  
>    //
> 

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Zeng, Star]

Share some information here according to my knowledge.

The EFI_SMM_BASE2_PROTOCOL.InSmm definition in PI spec is really very confusion. The naming for it are not consistent.
The interface name: In*Smm*
The typedef name of InSmm: EFI_*SMM_INSIDE_OUT*2
The second parameter name of InSmm: In*Smram*
 
In reality, the implementation of EFI_SMM_BASE2_PROTOCOL.InSmm in PiSmmIpl returns the information that whether current executing code is SMM code or executed from SMM code.

DXE > (SMM communcation) > InSmm = TRUE > SMM driver dispatcher/SMM handler > (exit SMM communication) > InSmm = FALSE > DXE


Thanks,
Star

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, June 11, 2018 8:18 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

Hi Jian,

On 06/11/18 09:08, Jian J Wang wrote:
> The SMM version of MemoryAllocationLib allows to free memory allocated 
> in DXE (before EndOfDxe). This is done by checking the memory range 
> and calling gBS services to do real operation if the memory to free is 
> out of SMRAM. This would cause problem if some memory related 
> features, like Heap Guard, have to update page table to change memory attributes.
> Because page table in SMM mode is different from DXE mode, gBS memory 
> services cannot get the correct attributes of DXE memory from SMM page 
> table and then cause incorrect memory manipulations.

(1) I think this description makes sense, but it does not highlight the involvement of CpuDxe.

(1a) Please reference "MdePkg/Library/SmmMemoryAllocationLib" specifically.

(1b) The SmmMemoryAllocationLib instance implies that the call chain starts with a DXE_SMM_DRIVER calling FreePool() or FreePages().

DXE_SMM_DRIVERs can only call boot services, and protocols located with boot services, in their entry point functions.

CpuDxe is a DXE_DRIVER and it provides the CPU arch protocol (and the MP services protocol). Thus, our call chain can only occur if:

- a DXE_SMM_DRIVER calls the FreePool() or FreePages() library API,

- in its entry point function,

- releasing memory that's not SMRAM,

- and the heap guard feature is enabled, so the FreePool() or
  FreePages() boot service ends up calling
  EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes().

Please include this information in the commit message.

(2) Can you remind me how the SMM page tables map non-SMRAM memory?

Because, I assume that, after a FreePages() libary API call, the freed memory should not be accessible to either SMM code or normal DXE code.
This seems to imply that both sets of page tables should be modified.
What am I missing?

> 
> The solution in this patch is to store the DXE page table information 
> (e.g. value of CR0, CR3 registers, etc.) in a global variable of 
> CpuDxe driver. If CpuDxe detects it's in SMM mode, it will use this 
> global variable to access page table instead of current processor registers.
> 
> Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f

(3) I think this comes from Gerrit. Do you really need the "Change-Id"
tag in the commit message? It doesn't say anything to the upstream edk2 users.

If you really need it, I don't mind it though; just please let's not add it due to oversight.

(4) Please make sure that you don't *push* the patch with Gerrit. Gerrit does bad things to git metadata. Please see <http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.ccr.corp.intel.com>.

> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c       |   2 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.inf     |   1 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 108 
> ++++++++++++++++++++++++++-------------
>  3 files changed, 75 insertions(+), 36 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c 
> index 6ae2dcd1c7..1fd996fc3f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -404,7 +404,7 @@ CpuSetMemoryAttributes (
>    // to avoid unnecessary computing.
>    //
>    if (mIsFlushingGCD) {
> -    DEBUG((DEBUG_INFO, "  Flushing GCD\n"));
> +    DEBUG((DEBUG_GCD, "  Flushing GCD\n"));
>      return EFI_SUCCESS;
>    }

(5) I think it would be cleaner if we fixed the debug level for this message in a separate patch. You are not touching "CpuDxe.c" for any other reason.

>  
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf 
> b/UefiCpuPkg/CpuDxe/CpuDxe.inf index 3c938cee53..8c8773af90 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -66,6 +66,7 @@
>  [Protocols]
>    gEfiCpuArchProtocolGuid                       ## PRODUCES
>    gEfiMpServiceProtocolGuid                     ## PRODUCES
> +  gEfiSmmBase2ProtocolGuid

(6) protocol usage note is missing (should be "## SOMETIMES_CONSUMES" I
think.)

>  
>  [Guids]
>    gIdleLoopEventGuid                            ## CONSUMES           ## Event
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c 
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index e2595b4d89..bf420d3792 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -23,6 +23,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Protocol/MpService.h>
> +#include <Protocol/SmmBase2.h>
>  
>  #include "CpuDxe.h"
>  #include "CpuPageTable.h"
> @@ -87,7 +88,33 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>    {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},  };
>  
> -PAGE_TABLE_POOL   *mPageTablePool = NULL;
> +PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
> +EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
> +
> +BOOLEAN
> +IsInSmm (
> +  VOID
> +  )
> +{
> +  EFI_STATUS              Status;
> +  BOOLEAN                 InSmm;
> +
> +  InSmm = FALSE;
> +  if (mSmmBase2 == NULL) {
> +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
> +                                  (VOID **)&mSmmBase2);

(7) Indentation is incorrect.

> +    if (EFI_ERROR (Status)) {
> +      mSmmBase2 = NULL;
> +    }

(8) I think you can simply drop the EFI_ERROR (Status) branch; "mSmmBase2" will simply remain NULL.

> +  }
> +
> +  if (mSmmBase2 != NULL) {
> +    mSmmBase2->InSmm (mSmmBase2, &InSmm);  }
> +
> +  return InSmm;
> +}

(9) I don't understand how the InSmm() member function is supposed to work. According to the PI spec (v1.6):

"""
EFI_MM_BASE_PROTOCOL.InMm()
[...]
Service to indicate whether the driver is currently executing in the MM Driver Initialization phase.
[...]
InMmram
Pointer to a Boolean which, on return, indicates that the driver is currently executing inside of MMRAM (TRUE) or outside of MMRAM (FALSE).
[...]
This service returns whether the caller is being executed in the MM Driver Initialization phase. For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE. For combination MM/DXE drivers, this will return FALSE in the DXE launch.
For the MM launch, it behaves as an MM Driver.
"""

First of all, to my knowledge, we don't have any combination MM/DXE drivers in edk2 (with dual entry point launches, binary type 0x0C). We have traditional MM drivers (binary type 0x0A) that are launched directly from SMRAM. However, it is not guaranteed that they are launched with the processor operating in System Management Mode.

Secondly, focusing on traditional MM drivers, the InMm() specification doesn't seem to say anything as to whether the processor is running in SMM (system management mode) or normal mode. Initially, SMRAM is open, and thus the processor can execute MM driver code out of it without actually operating in SMM (see above).

Furthermore, CpuDxe is a DXE_DRIVER, thus it is never loaded into SMRAM.

I notice that you call the "InMmram" parameter "InSmm". Are you saying that the PI spec is wrong, and edk2 actually returns whether the processor is executing CpuDxe code in Management Mode? (Due to a call from the entry point function of a traditional MM driver?)

Third, the spec is inconsistent even with itself. The general description says, "For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE". But a DXE protocol must not even be invoked from a traditional MM driver *except* from the entry point! Which would imply that, for traditional MM drivers, the output parameter could only be set to TRUE.

So basically what I'd like to see here is evidence that

(9a) the IsInSmm() helper function is never called outside of the entry
     point of an MM driver, due to the restrictions on
     gBS->LocateProtocol() and mSmmBase2->InSmm(),

(9b) the mSmmBase2->InSmm() protocol member determines whether CpuDxe is
     being executed by the processor in Management Mode, and *not*
     whether the CpuDxe driver is being executed from within SMRAM.

>  
>  /**
>    Return current paging context.
> @@ -102,42 +129,45 @@ GetCurrentPagingContext (
>    UINT32                         RegEax;
>    UINT32                         RegEdx;
>  
> -  ZeroMem(PagingContext, sizeof(*PagingContext));
> -  if (sizeof(UINTN) == sizeof(UINT64)) {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_X64;
> -  } else {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_I386;
> -  }
> -  if ((AsmReadCr0 () & BIT31) != 0) {
> -    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> -  } else {
> -    PagingContext->ContextData.X64.PageTableBase = 0;
> -  }
> +  if (!IsInSmm ()) {
> +    if (sizeof(UINTN) == sizeof(UINT64)) {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> +    } else {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> +    }
> +    if ((AsmReadCr0 () & BIT31) != 0) {
> +      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> +    } else {
> +      mPagingContext.ContextData.X64.PageTableBase = 0;
> +    }
>  
> -  if ((AsmReadCr4 () & BIT4) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> -  }
> -  if ((AsmReadCr4 () & BIT5) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> -  }
> -  if ((AsmReadCr0 () & BIT16) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> -  }
> +    if ((AsmReadCr4 () & BIT4) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> +    }
> +    if ((AsmReadCr4 () & BIT5) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> +    }
> +    if ((AsmReadCr0 () & BIT16) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> +    }
>  
> -  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> -  if (RegEax > 0x80000000) {
> -    AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> -    if ((RegEdx & BIT20) != 0) {
> -      // XD supported
> -      if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> -        // XD activated
> -        PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> +    if (RegEax > 0x80000000) {
> +      AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> +      if ((RegEdx & BIT20) != 0) {
> +        // XD supported
> +        if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> +          // XD activated
> +          mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +        }
> +      }
> +      if ((RegEdx & BIT26) != 0) {
> +        mPagingContext.ContextData.Ia32.Attributes |= 
> + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>        }
> -    }
> -    if ((RegEdx & BIT26) != 0) {
> -      PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>      }
>    }
> +
> +  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
>  }

(10) Even assuming that IsInSmm() always returns false -- for example because the platform does not include the SMM drivers --, this change does not look like a no-op to me.

The ZeroMem() call at the top is removed by the patch, but attributes like PSE, PAE, WP_ENABLE, XD_ACTIVATED and 1G_SUPPORT are never
*cleared* in "mPagingContext", once they are set.

I guess the idea is that a few (how many?) initial calls to
GetCurrentPagingContext() set up "mPagingContext", and after that,
GetCurrentPagingContext() just keeps returning the last set context. But I don't think that those "initial calls" work correctly.

>  
>  /**
> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
>    VOID
>    )
>  {
> -  return ((AsmReadCr0 () & BIT16) != 0);
> +  if (!IsInSmm ()) {
> +    return ((AsmReadCr0 () & BIT16) != 0);  }  return FALSE;
>  }
>  
>  /**
> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);  }
>  }
>  
>  /**
> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () | BIT16);
> +  }
>  }

(11) I don't understand these changes at all. Why are these functions
no-ops when CpuDxe is invoked in Management Mode?

Thanks
Laszlo

>  
>  /**
> @@ -1054,6 +1091,7 @@ InitializePageTableLib (
>  {
>    PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
>  
> +  ZeroMem (&mPagingContext, sizeof(mPagingContext));
>    GetCurrentPagingContext (&CurrentPagingContext);
>  
>    //
> 

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

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Zeng, Star]

Sorry, fix typo.

DXE > (SMM communcation) > InSmm = TRUE > SMM driver dispatcher/SMM handler > InSmm = FALSE > (exit SMM communication) > DXE

-----Original Message-----
From: Zeng, Star 
Sent: Tuesday, June 12, 2018 11:35 AM
To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

Share some information here according to my knowledge.

The EFI_SMM_BASE2_PROTOCOL.InSmm definition in PI spec is really very confusion. The naming for it are not consistent.
The interface name: In*Smm*
The typedef name of InSmm: EFI_*SMM_INSIDE_OUT*2 The second parameter name of InSmm: In*Smram*
 
In reality, the implementation of EFI_SMM_BASE2_PROTOCOL.InSmm in PiSmmIpl returns the information that whether current executing code is SMM code or executed from SMM code.

DXE > (SMM communcation) > InSmm = TRUE > SMM driver dispatcher/SMM handler > (exit SMM communication) > InSmm = FALSE > DXE


Thanks,
Star

-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Monday, June 11, 2018 8:18 PM
To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>
Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

Hi Jian,

On 06/11/18 09:08, Jian J Wang wrote:
> The SMM version of MemoryAllocationLib allows to free memory allocated 
> in DXE (before EndOfDxe). This is done by checking the memory range 
> and calling gBS services to do real operation if the memory to free is 
> out of SMRAM. This would cause problem if some memory related 
> features, like Heap Guard, have to update page table to change memory attributes.
> Because page table in SMM mode is different from DXE mode, gBS memory 
> services cannot get the correct attributes of DXE memory from SMM page 
> table and then cause incorrect memory manipulations.

(1) I think this description makes sense, but it does not highlight the involvement of CpuDxe.

(1a) Please reference "MdePkg/Library/SmmMemoryAllocationLib" specifically.

(1b) The SmmMemoryAllocationLib instance implies that the call chain starts with a DXE_SMM_DRIVER calling FreePool() or FreePages().

DXE_SMM_DRIVERs can only call boot services, and protocols located with boot services, in their entry point functions.

CpuDxe is a DXE_DRIVER and it provides the CPU arch protocol (and the MP services protocol). Thus, our call chain can only occur if:

- a DXE_SMM_DRIVER calls the FreePool() or FreePages() library API,

- in its entry point function,

- releasing memory that's not SMRAM,

- and the heap guard feature is enabled, so the FreePool() or
  FreePages() boot service ends up calling
  EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes().

Please include this information in the commit message.

(2) Can you remind me how the SMM page tables map non-SMRAM memory?

Because, I assume that, after a FreePages() libary API call, the freed memory should not be accessible to either SMM code or normal DXE code.
This seems to imply that both sets of page tables should be modified.
What am I missing?

> 
> The solution in this patch is to store the DXE page table information 
> (e.g. value of CR0, CR3 registers, etc.) in a global variable of 
> CpuDxe driver. If CpuDxe detects it's in SMM mode, it will use this 
> global variable to access page table instead of current processor registers.
> 
> Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f

(3) I think this comes from Gerrit. Do you really need the "Change-Id"
tag in the commit message? It doesn't say anything to the upstream edk2 users.

If you really need it, I don't mind it though; just please let's not add it due to oversight.

(4) Please make sure that you don't *push* the patch with Gerrit. Gerrit does bad things to git metadata. Please see <http://mid.mail-archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.ccr.corp.intel.com>.

> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>  UefiCpuPkg/CpuDxe/CpuDxe.c       |   2 +-
>  UefiCpuPkg/CpuDxe/CpuDxe.inf     |   1 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 108
> ++++++++++++++++++++++++++-------------
>  3 files changed, 75 insertions(+), 36 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c 
> index 6ae2dcd1c7..1fd996fc3f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -404,7 +404,7 @@ CpuSetMemoryAttributes (
>    // to avoid unnecessary computing.
>    //
>    if (mIsFlushingGCD) {
> -    DEBUG((DEBUG_INFO, "  Flushing GCD\n"));
> +    DEBUG((DEBUG_GCD, "  Flushing GCD\n"));
>      return EFI_SUCCESS;
>    }

(5) I think it would be cleaner if we fixed the debug level for this message in a separate patch. You are not touching "CpuDxe.c" for any other reason.

>  
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf 
> b/UefiCpuPkg/CpuDxe/CpuDxe.inf index 3c938cee53..8c8773af90 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -66,6 +66,7 @@
>  [Protocols]
>    gEfiCpuArchProtocolGuid                       ## PRODUCES
>    gEfiMpServiceProtocolGuid                     ## PRODUCES
> +  gEfiSmmBase2ProtocolGuid

(6) protocol usage note is missing (should be "## SOMETIMES_CONSUMES" I
think.)

>  
>  [Guids]
>    gIdleLoopEventGuid                            ## CONSUMES           ## Event
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index e2595b4d89..bf420d3792 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -23,6 +23,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Protocol/MpService.h>
> +#include <Protocol/SmmBase2.h>
>  
>  #include "CpuDxe.h"
>  #include "CpuPageTable.h"
> @@ -87,7 +88,33 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
>    {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},  };
>  
> -PAGE_TABLE_POOL   *mPageTablePool = NULL;
> +PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> +PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
> +EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
> +
> +BOOLEAN
> +IsInSmm (
> +  VOID
> +  )
> +{
> +  EFI_STATUS              Status;
> +  BOOLEAN                 InSmm;
> +
> +  InSmm = FALSE;
> +  if (mSmmBase2 == NULL) {
> +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
> +                                  (VOID **)&mSmmBase2);

(7) Indentation is incorrect.

> +    if (EFI_ERROR (Status)) {
> +      mSmmBase2 = NULL;
> +    }

(8) I think you can simply drop the EFI_ERROR (Status) branch; "mSmmBase2" will simply remain NULL.

> +  }
> +
> +  if (mSmmBase2 != NULL) {
> +    mSmmBase2->InSmm (mSmmBase2, &InSmm);  }
> +
> +  return InSmm;
> +}

(9) I don't understand how the InSmm() member function is supposed to work. According to the PI spec (v1.6):

"""
EFI_MM_BASE_PROTOCOL.InMm()
[...]
Service to indicate whether the driver is currently executing in the MM Driver Initialization phase.
[...]
InMmram
Pointer to a Boolean which, on return, indicates that the driver is currently executing inside of MMRAM (TRUE) or outside of MMRAM (FALSE).
[...]
This service returns whether the caller is being executed in the MM Driver Initialization phase. For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE. For combination MM/DXE drivers, this will return FALSE in the DXE launch.
For the MM launch, it behaves as an MM Driver.
"""

First of all, to my knowledge, we don't have any combination MM/DXE drivers in edk2 (with dual entry point launches, binary type 0x0C). We have traditional MM drivers (binary type 0x0A) that are launched directly from SMRAM. However, it is not guaranteed that they are launched with the processor operating in System Management Mode.

Secondly, focusing on traditional MM drivers, the InMm() specification doesn't seem to say anything as to whether the processor is running in SMM (system management mode) or normal mode. Initially, SMRAM is open, and thus the processor can execute MM driver code out of it without actually operating in SMM (see above).

Furthermore, CpuDxe is a DXE_DRIVER, thus it is never loaded into SMRAM.

I notice that you call the "InMmram" parameter "InSmm". Are you saying that the PI spec is wrong, and edk2 actually returns whether the processor is executing CpuDxe code in Management Mode? (Due to a call from the entry point function of a traditional MM driver?)

Third, the spec is inconsistent even with itself. The general description says, "For MM Drivers, this will return TRUE in InMmram while inside the driver’s entry point and otherwise FALSE". But a DXE protocol must not even be invoked from a traditional MM driver *except* from the entry point! Which would imply that, for traditional MM drivers, the output parameter could only be set to TRUE.

So basically what I'd like to see here is evidence that

(9a) the IsInSmm() helper function is never called outside of the entry
     point of an MM driver, due to the restrictions on
     gBS->LocateProtocol() and mSmmBase2->InSmm(),

(9b) the mSmmBase2->InSmm() protocol member determines whether CpuDxe is
     being executed by the processor in Management Mode, and *not*
     whether the CpuDxe driver is being executed from within SMRAM.

>  
>  /**
>    Return current paging context.
> @@ -102,42 +129,45 @@ GetCurrentPagingContext (
>    UINT32                         RegEax;
>    UINT32                         RegEdx;
>  
> -  ZeroMem(PagingContext, sizeof(*PagingContext));
> -  if (sizeof(UINTN) == sizeof(UINT64)) {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_X64;
> -  } else {
> -    PagingContext->MachineType = IMAGE_FILE_MACHINE_I386;
> -  }
> -  if ((AsmReadCr0 () & BIT31) != 0) {
> -    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> -  } else {
> -    PagingContext->ContextData.X64.PageTableBase = 0;
> -  }
> +  if (!IsInSmm ()) {
> +    if (sizeof(UINTN) == sizeof(UINT64)) {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> +    } else {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> +    }
> +    if ((AsmReadCr0 () & BIT31) != 0) {
> +      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> +    } else {
> +      mPagingContext.ContextData.X64.PageTableBase = 0;
> +    }
>  
> -  if ((AsmReadCr4 () & BIT4) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> -  }
> -  if ((AsmReadCr4 () & BIT5) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> -  }
> -  if ((AsmReadCr0 () & BIT16) != 0) {
> -    PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> -  }
> +    if ((AsmReadCr4 () & BIT4) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> +    }
> +    if ((AsmReadCr4 () & BIT5) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> +    }
> +    if ((AsmReadCr0 () & BIT16) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> +    }
>  
> -  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> -  if (RegEax > 0x80000000) {
> -    AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> -    if ((RegEdx & BIT20) != 0) {
> -      // XD supported
> -      if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> -        // XD activated
> -        PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> +    if (RegEax > 0x80000000) {
> +      AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> +      if ((RegEdx & BIT20) != 0) {
> +        // XD supported
> +        if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> +          // XD activated
> +          mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +        }
> +      }
> +      if ((RegEdx & BIT26) != 0) {
> +        mPagingContext.ContextData.Ia32.Attributes |= 
> + PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>        }
> -    }
> -    if ((RegEdx & BIT26) != 0) {
> -      PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
>      }
>    }
> +
> +  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
>  }

(10) Even assuming that IsInSmm() always returns false -- for example because the platform does not include the SMM drivers --, this change does not look like a no-op to me.

The ZeroMem() call at the top is removed by the patch, but attributes like PSE, PAE, WP_ENABLE, XD_ACTIVATED and 1G_SUPPORT are never
*cleared* in "mPagingContext", once they are set.

I guess the idea is that a few (how many?) initial calls to
GetCurrentPagingContext() set up "mPagingContext", and after that,
GetCurrentPagingContext() just keeps returning the last set context. But I don't think that those "initial calls" work correctly.

>  
>  /**
> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
>    VOID
>    )
>  {
> -  return ((AsmReadCr0 () & BIT16) != 0);
> +  if (!IsInSmm ()) {
> +    return ((AsmReadCr0 () & BIT16) != 0);  }  return FALSE;
>  }
>  
>  /**
> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);  }
>  }
>  
>  /**
> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () | BIT16);  }
>  }

(11) I don't understand these changes at all. Why are these functions no-ops when CpuDxe is invoked in Management Mode?

Thanks
Laszlo

>  
>  /**
> @@ -1054,6 +1091,7 @@ InitializePageTableLib (  {
>    PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
>  
> +  ZeroMem (&mPagingContext, sizeof(mPagingContext));
>    GetCurrentPagingContext (&CurrentPagingContext);
>  
>    //
> 

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

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Laszlo Ersek]

On 06/12/18 05:36, Zeng, Star wrote:
> Sorry, fix typo.
> 
> DXE > (SMM communcation) > InSmm = TRUE > SMM driver dispatcher/SMM handler > InSmm = FALSE > (exit SMM communication) > DXE

Thank you Star, I'll have to remember this!

Laszlo

> -----Original Message-----
> From: Zeng, Star 
> Sent: Tuesday, June 12, 2018 11:35 AM
> To: Laszlo Ersek <lersek@redhat.com>; Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: RE: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode
> 
> Share some information here according to my knowledge.
> 
> The EFI_SMM_BASE2_PROTOCOL.InSmm definition in PI spec is really very confusion. The naming for it are not consistent.
> The interface name: In*Smm*
> The typedef name of InSmm: EFI_*SMM_INSIDE_OUT*2 The second parameter name of InSmm: In*Smram*
>  
> In reality, the implementation of EFI_SMM_BASE2_PROTOCOL.InSmm in PiSmmIpl returns the information that whether current executing code is SMM code or executed from SMM code.
> 
> DXE > (SMM communcation) > InSmm = TRUE > SMM driver dispatcher/SMM handler > (exit SMM communication) > InSmm = FALSE > DXE

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Wang, Jian J]

Hi Laszlo,

Thank you very much for such thorough review. I'd like to explain a bit in advance.

Putting aside the specific coding issues in my patch, one thing is clear that SMM mode
has its own page table. CpuDxe should not touch it even if its public interface is called
via gBS services, because it has no knowledge of SMM internal things and it might
jeopardize SMM operation if it does. But current CpuDxe doesn't take this into account.
I think it should be fixed anyway, even there's no Heap Guard feature, since current
design allows SMM to touch memory owned by DXE but not vice versa.

The basic idea of this patch is, if we want to access DXE page table in SMM mode, we
cannot do this via CR3 register (it has been reloaded by SMM) directly but via a stored
value in a global. This is feasible because page table is just a chunk of normal memory.
All we need is an entry address pointer. When exiting SMM mode back to DXE, the
updated page table will take effect immediately once CR3 is restored to DXE version.
That means we can change DXE page table attributes even in SMM mode.

Please see my other responses below.

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, June 11, 2018 8:18 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong,
> Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE)
> page table in SMM mode
> 
> Hi Jian,
> 
> On 06/11/18 09:08, Jian J Wang wrote:
> > The SMM version of MemoryAllocationLib allows to free memory allocated
> > in DXE (before EndOfDxe). This is done by checking the memory range and
> > calling gBS services to do real operation if the memory to free is out
> > of SMRAM. This would cause problem if some memory related features, like
> > Heap Guard, have to update page table to change memory attributes.
> > Because page table in SMM mode is different from DXE mode, gBS memory
> > services cannot get the correct attributes of DXE memory from SMM page
> > table and then cause incorrect memory manipulations.
> 
> (1) I think this description makes sense, but it does not highlight the
> involvement of CpuDxe.
> 
> (1a) Please reference "MdePkg/Library/SmmMemoryAllocationLib" specifically.
> 
> (1b) The SmmMemoryAllocationLib instance implies that the call chain
> starts with a DXE_SMM_DRIVER calling FreePool() or FreePages().
> 
> DXE_SMM_DRIVERs can only call boot services, and protocols located with
> boot services, in their entry point functions.
> 
> CpuDxe is a DXE_DRIVER and it provides the CPU arch protocol (and the MP
> services protocol). Thus, our call chain can only occur if:
> 
> - a DXE_SMM_DRIVER calls the FreePool() or FreePages() library API,
> 
> - in its entry point function,
> 
> - releasing memory that's not SMRAM,
> 
> - and the heap guard feature is enabled, so the FreePool() or
>   FreePages() boot service ends up calling
>   EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes().
> 
> Please include this information in the commit message.
> 

Sure. I'll add it. Thanks.

> (2) Can you remind me how the SMM page tables map non-SMRAM memory?
> 
> Because, I assume that, after a FreePages() libary API call, the freed
> memory should not be accessible to either SMM code or normal DXE code.
> This seems to imply that both sets of page tables should be modified.
> What am I missing?
> 

For both SMM and DXE, we just do 1:1 full memory mapping in paging but in
different page tables. As far as I know, there's no such rules that freed pages
should not be accessible. I think it's just implementation dependent.

> >
> > The solution in this patch is to store the DXE page table information
> > (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe
> > driver. If CpuDxe detects it's in SMM mode, it will use this global
> > variable to access page table instead of current processor registers.
> >
> > Change-Id: If810bb1828160b8bdd8cb616d86df2859c74971f
> 
> (3) I think this comes from Gerrit. Do you really need the "Change-Id"
> tag in the commit message? It doesn't say anything to the upstream edk2
> users.
> 
> If you really need it, I don't mind it though; just please let's not add
> it due to oversight.
> 

Sorry, it's added by our internal tool automatically. It can be deleted.

> (4) Please make sure that you don't *push* the patch with Gerrit. Gerrit
> does bad things to git metadata. Please see
> <http://mid.mail-
> archive.com/4A89E2EF3DFEDB4C8BFDE51014F606A14E22344D@SHSMSX104.c
> cr.corp.intel.com>.
> 

Sure.

> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> > ---
> >  UefiCpuPkg/CpuDxe/CpuDxe.c       |   2 +-
> >  UefiCpuPkg/CpuDxe/CpuDxe.inf     |   1 +
> >  UefiCpuPkg/CpuDxe/CpuPageTable.c | 108 ++++++++++++++++++++++++++---
> ----------
> >  3 files changed, 75 insertions(+), 36 deletions(-)
> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> > index 6ae2dcd1c7..1fd996fc3f 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> > @@ -404,7 +404,7 @@ CpuSetMemoryAttributes (
> >    // to avoid unnecessary computing.
> >    //
> >    if (mIsFlushingGCD) {
> > -    DEBUG((DEBUG_INFO, "  Flushing GCD\n"));
> > +    DEBUG((DEBUG_GCD, "  Flushing GCD\n"));
> >      return EFI_SUCCESS;
> >    }
> 
> (5) I think it would be cleaner if we fixed the debug level for this
> message in a separate patch. You are not touching "CpuDxe.c" for any
> other reason.
> 

My fault. I forgot to restore it. It's just for my test.

> >
> > diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > index 3c938cee53..8c8773af90 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> > @@ -66,6 +66,7 @@
> >  [Protocols]
> >    gEfiCpuArchProtocolGuid                       ## PRODUCES
> >    gEfiMpServiceProtocolGuid                     ## PRODUCES
> > +  gEfiSmmBase2ProtocolGuid
> 
> (6) protocol usage note is missing (should be "## SOMETIMES_CONSUMES" I
> think.)

You're right. I'll add it.

> 
> >
> >  [Guids]
> >    gIdleLoopEventGuid                            ## CONSUMES           ## Event
> > diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > index e2595b4d89..bf420d3792 100644
> > --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> > @@ -23,6 +23,7 @@
> >  #include <Library/DebugLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >  #include <Protocol/MpService.h>
> > +#include <Protocol/SmmBase2.h>
> >
> >  #include "CpuDxe.h"
> >  #include "CpuPageTable.h"
> > @@ -87,7 +88,33 @@ PAGE_ATTRIBUTE_TABLE mPageAttributeTable[] = {
> >    {Page1G,  SIZE_1GB, PAGING_1G_ADDRESS_MASK_64},
> >  };
> >
> > -PAGE_TABLE_POOL   *mPageTablePool = NULL;
> > +PAGE_TABLE_POOL                   *mPageTablePool = NULL;
> > +PAGE_TABLE_LIB_PAGING_CONTEXT     mPagingContext;
> > +EFI_SMM_BASE2_PROTOCOL            *mSmmBase2 = NULL;
> > +
> > +BOOLEAN
> > +IsInSmm (
> > +  VOID
> > +  )
> > +{
> > +  EFI_STATUS              Status;
> > +  BOOLEAN                 InSmm;
> > +
> > +  InSmm = FALSE;
> > +  if (mSmmBase2 == NULL) {
> > +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
> > +                                  (VOID **)&mSmmBase2);
> 
> (7) Indentation is incorrect.
> 

It'll be fixed.

> > +    if (EFI_ERROR (Status)) {
> > +      mSmmBase2 = NULL;
> > +    }
> 
> (8) I think you can simply drop the EFI_ERROR (Status) branch;
> "mSmmBase2" will simply remain NULL.
> 
> > +  }
> > +
> > +  if (mSmmBase2 != NULL) {
> > +    mSmmBase2->InSmm (mSmmBase2, &InSmm);
> > +  }
> > +
> > +  return InSmm;
> > +}
> 
> (9) I don't understand how the InSmm() member function is supposed to
> work. According to the PI spec (v1.6):
> 
> """
> EFI_MM_BASE_PROTOCOL.InMm()
> [...]
> Service to indicate whether the driver is currently executing in the MM
> Driver Initialization phase.
> [...]
> InMmram
> Pointer to a Boolean which, on return, indicates that the driver is
> currently executing inside of MMRAM (TRUE) or outside of MMRAM (FALSE).
> [...]
> This service returns whether the caller is being executed in the MM
> Driver Initialization phase. For MM Drivers, this will return TRUE in
> InMmram while inside the driver’s entry point and otherwise FALSE. For
> combination MM/DXE drivers, this will return FALSE in the DXE launch.
> For the MM launch, it behaves as an MM Driver.
> """
> 
> First of all, to my knowledge, we don't have any combination MM/DXE
> drivers in edk2 (with dual entry point launches, binary type 0x0C). We
> have traditional MM drivers (binary type 0x0A) that are launched
> directly from SMRAM. However, it is not guaranteed that they are
> launched with the processor operating in System Management Mode.
> 
> Secondly, focusing on traditional MM drivers, the InMm() specification
> doesn't seem to say anything as to whether the processor is running in
> SMM (system management mode) or normal mode. Initially, SMRAM is open,
> and thus the processor can execute MM driver code out of it without
> actually operating in SMM (see above).
> 
> Furthermore, CpuDxe is a DXE_DRIVER, thus it is never loaded into SMRAM.
> 
> I notice that you call the "InMmram" parameter "InSmm". Are you saying
> that the PI spec is wrong, and edk2 actually returns whether the
> processor is executing CpuDxe code in Management Mode? (Due to a call
> from the entry point function of a traditional MM driver?)
> 
> Third, the spec is inconsistent even with itself. The general
> description says, "For MM Drivers, this will return TRUE in InMmram
> while inside the driver’s entry point and otherwise FALSE". But a DXE
> protocol must not even be invoked from a traditional MM driver *except*
> from the entry point! Which would imply that, for traditional MM
> drivers, the output parameter could only be set to TRUE.
> 
> So basically what I'd like to see here is evidence that
> 
> (9a) the IsInSmm() helper function is never called outside of the entry
>      point of an MM driver, due to the restrictions on
>      gBS->LocateProtocol() and mSmmBase2->InSmm(),
> 
> (9b) the mSmmBase2->InSmm() protocol member determines whether CpuDxe is
>      being executed by the processor in Management Mode, and *not*
>      whether the CpuDxe driver is being executed from within SMRAM.
> 

Please refer to Star's comments.

> >
> >  /**
> >    Return current paging context.
> > @@ -102,42 +129,45 @@ GetCurrentPagingContext (
> >    UINT32                         RegEax;
> >    UINT32                         RegEdx;
> >
> > -  ZeroMem(PagingContext, sizeof(*PagingContext));
> > -  if (sizeof(UINTN) == sizeof(UINT64)) {
> > -    PagingContext->MachineType = IMAGE_FILE_MACHINE_X64;
> > -  } else {
> > -    PagingContext->MachineType = IMAGE_FILE_MACHINE_I386;
> > -  }
> > -  if ((AsmReadCr0 () & BIT31) != 0) {
> > -    PagingContext->ContextData.X64.PageTableBase = (AsmReadCr3 () &
> PAGING_4K_ADDRESS_MASK_64);
> > -  } else {
> > -    PagingContext->ContextData.X64.PageTableBase = 0;
> > -  }
> > +  if (!IsInSmm ()) {
> > +    if (sizeof(UINTN) == sizeof(UINT64)) {
> > +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> > +    } else {
> > +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> > +    }
> > +    if ((AsmReadCr0 () & BIT31) != 0) {
> > +      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () &
> PAGING_4K_ADDRESS_MASK_64);
> > +    } else {
> > +      mPagingContext.ContextData.X64.PageTableBase = 0;
> > +    }
> >
> > -  if ((AsmReadCr4 () & BIT4) != 0) {
> > -    PagingContext->ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> > -  }
> > -  if ((AsmReadCr4 () & BIT5) != 0) {
> > -    PagingContext->ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> > -  }
> > -  if ((AsmReadCr0 () & BIT16) != 0) {
> > -    PagingContext->ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> > -  }
> > +    if ((AsmReadCr4 () & BIT4) != 0) {
> > +      mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> > +    }
> > +    if ((AsmReadCr4 () & BIT5) != 0) {
> > +      mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> > +    }
> > +    if ((AsmReadCr0 () & BIT16) != 0) {
> > +      mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
> > +    }
> >
> > -  AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> > -  if (RegEax > 0x80000000) {
> > -    AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> > -    if ((RegEdx & BIT20) != 0) {
> > -      // XD supported
> > -      if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> > -        // XD activated
> > -        PagingContext->ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> > +    AsmCpuid (0x80000000, &RegEax, NULL, NULL, NULL);
> > +    if (RegEax > 0x80000000) {
> > +      AsmCpuid (0x80000001, NULL, NULL, NULL, &RegEdx);
> > +      if ((RegEdx & BIT20) != 0) {
> > +        // XD supported
> > +        if ((AsmReadMsr64 (0xC0000080) & BIT11) != 0) {
> > +          // XD activated
> > +          mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> > +        }
> > +      }
> > +      if ((RegEdx & BIT26) != 0) {
> > +        mPagingContext.ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPO
> RT;
> >        }
> > -    }
> > -    if ((RegEdx & BIT26) != 0) {
> > -      PagingContext->ContextData.Ia32.Attributes |=
> PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPO
> RT;
> >      }
> >    }
> > +
> > +  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
> >  }
> 
> (10) Even assuming that IsInSmm() always returns false -- for example
> because the platform does not include the SMM drivers --, this change
> does not look like a no-op to me.
> 
> The ZeroMem() call at the top is removed by the patch, but attributes
> like PSE, PAE, WP_ENABLE, XD_ACTIVATED and 1G_SUPPORT are never
> *cleared* in "mPagingContext", once they are set.
> 
> I guess the idea is that a few (how many?) initial calls to
> GetCurrentPagingContext() set up "mPagingContext", and after that,
> GetCurrentPagingContext() just keeps returning the last set context. But
> I don't think that those "initial calls" work correctly.
> 

You're right. Thanks for pointing it out.
 
> >
> >  /**
> > @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
> >    VOID
> >    )
> >  {
> > -  return ((AsmReadCr0 () & BIT16) != 0);
> > +  if (!IsInSmm ()) {
> > +    return ((AsmReadCr0 () & BIT16) != 0);
> > +  }
> > +  return FALSE;
> >  }
> >
> >  /**
> > @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
> >    VOID
> >    )
> >  {
> > -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> > +  if (!IsInSmm ()) {
> > +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);
> > +  }
> >  }
> >
> >  /**
> > @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
> >    VOID
> >    )
> >  {
> > -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> > +  if (!IsInSmm ()) {
> > +    AsmWriteCr0 (AsmReadCr0 () | BIT16);
> > +  }
> >  }
> 
> (11) I don't understand these changes at all. Why are these functions
> no-ops when CpuDxe is invoked in Management Mode?
> 

CR0.BIT16 is used to protect current (active) page table. In CpuDxe, we should
not touch SMM page table if in SMM mode. Although it looks like there's no
problem to set this bit anyway because we just want to access DXE page table,
I don't want to risk to expose any potential issues here. So I think it may be safer
not to touch any SMM paging settings from non-SMM code.

> Thanks
> Laszlo
> 
> >
> >  /**
> > @@ -1054,6 +1091,7 @@ InitializePageTableLib (
> >  {
> >    PAGE_TABLE_LIB_PAGING_CONTEXT     CurrentPagingContext;
> >
> > +  ZeroMem (&mPagingContext, sizeof(mPagingContext));
> >    GetCurrentPagingContext (&CurrentPagingContext);
> >
> >    //
> >

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Laszlo Ersek]

On 06/12/18 06:32, Wang, Jian J wrote:
> Hi Laszlo,
> 
> Thank you very much for such thorough review. I'd like to explain a bit in advance.
> 
> Putting aside the specific coding issues in my patch, one thing is clear that SMM mode
> has its own page table. CpuDxe should not touch it even if its public interface is called
> via gBS services, because it has no knowledge of SMM internal things and it might
> jeopardize SMM operation if it does. But current CpuDxe doesn't take this into account.

Can you explain this in more detail? I mean, when CpuDxe protocol
members are called outside of SMM, CpuDxe obviously can't mess with SMM
page tables, because those are located in SMRAM, and non-SMM code cannot
access SMRAM (at that point).

So, I think you mean that, CpuDxe currently modifies whatever page
tables are pointed-to by CR3, even if CpuDxe protocol members are
(indirectly) invoked from code that runs in SMM. This results in CpuDxe
messing with SMM page tables that were not meant for CpuDxe to modify.
Is my understanding correct?

> I think it should be fixed anyway, even there's no Heap Guard feature,

OK, if my above summary is correct, then this makes sense to me.

> since current
> design allows SMM to touch memory owned by DXE but not vice versa.

Right; it is valid for a traditional SMM driver to allocate and free
both SMRAM and DXE memory in its entry point function.

> The basic idea of this patch is, if we want to access DXE page table in SMM mode, we
> cannot do this via CR3 register (it has been reloaded by SMM) directly but via a stored
> value in a global. This is feasible because page table is just a chunk of normal memory.
> All we need is an entry address pointer. When exiting SMM mode back to DXE, the
> updated page table will take effect immediately once CR3 is restored to DXE version.
> That means we can change DXE page table attributes even in SMM mode.

Makes sense.

> Please see my other responses below.

OK:

[snip]

>> (2) Can you remind me how the SMM page tables map non-SMRAM memory?
>>
>> Because, I assume that, after a FreePages() libary API call, the freed
>> memory should not be accessible to either SMM code or normal DXE code.
>> This seems to imply that both sets of page tables should be modified.
>> What am I missing?
>>
> 
> For both SMM and DXE, we just do 1:1 full memory mapping in paging but in
> different page tables. As far as I know, there's no such rules that freed pages
> should not be accessible. I think it's just implementation dependent.

Let's assume we have a DXE page allocation, with the heap guard enabled.
To my understanding, the allocated area is preceded and succeeded by 1
page on each end, and both of those pages are marked as inaccessible, so
that we get a page fault on buffer overflow / underflow.

Because traditional SMM drivers are allowed to work with DXE memory in
their entry point functions, I *believe* (I'm not sure) that the guard
pages should be marked as inaccessible in both the DXE page tables and
in the SMM page tables; so that buffer overflow/underflow trap in both
operating modes. Is my understanding correct?

FWIW, I based my question in (2) on the above understading -- if the
heap guard feature marks the guard pages inaccessible for both operating
modes at allocation time, then whatever PTE-level "undo" is performed at
gBS->FreePages() time, should likely also modify both sets of page tables.

Of course, if the DXE heap guard modifies the PTEs for the guard pages
only in the DXE page tables, at allocation time, then the SMM page
tables are irrelevant at release time as well.

[snip]

>>> +BOOLEAN
>>> +IsInSmm (
>>> +  VOID
>>> +  )
>>> +{
>>> +  EFI_STATUS              Status;
>>> +  BOOLEAN                 InSmm;
>>> +
>>> +  InSmm = FALSE;
>>> +  if (mSmmBase2 == NULL) {
>>> +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
>>> +                                  (VOID **)&mSmmBase2);
>>
>> (7) Indentation is incorrect.
>>
> 
> It'll be fixed.
> 
>>> +    if (EFI_ERROR (Status)) {
>>> +      mSmmBase2 = NULL;
>>> +    }
>>
>> (8) I think you can simply drop the EFI_ERROR (Status) branch;
>> "mSmmBase2" will simply remain NULL.

You didn't comment on this: do you agree, or is it necessary for some
reason to re-assign NULL to mSmmBase2? (For example, a static analyzer
or a compiler complains etc...)

[snip]

>>> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
>>>    VOID
>>>    )
>>>  {
>>> -  return ((AsmReadCr0 () & BIT16) != 0);
>>> +  if (!IsInSmm ()) {
>>> +    return ((AsmReadCr0 () & BIT16) != 0);
>>> +  }
>>> +  return FALSE;
>>>  }
>>>
>>>  /**
>>> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
>>>    VOID
>>>    )
>>>  {
>>> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
>>> +  if (!IsInSmm ()) {
>>> +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);
>>> +  }
>>>  }
>>>
>>>  /**
>>> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
>>>    VOID
>>>    )
>>>  {
>>> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
>>> +  if (!IsInSmm ()) {
>>> +    AsmWriteCr0 (AsmReadCr0 () | BIT16);
>>> +  }
>>>  }
>>
>> (11) I don't understand these changes at all. Why are these functions
>> no-ops when CpuDxe is invoked in Management Mode?
>>
> 
> CR0.BIT16 is used to protect current (active) page table.

The SDM writes,

    Write Protect (bit 16 of CR0) — When set, inhibits supervisor-level
    procedures from writing into read-only pages; when clear, allows
    supervisor-level procedures to write into read-only pages
    (regardless of the U/S bit setting; see Section 4.1.3 and Section
    4.6). This flag facilitates implementation of the copy-on-write
    method of creating a new process (forking) used by operating systems
    such as UNIX.

First, I think we should use a macro for "write protect" that is more
telling than just BIT16. We have "CR0_WP" in both
- MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h

so maybe we should move that to some IndustryStandard header. Should be
a separate patch / BZ anyway.

Second, from looking at the CpuDxe code, it seems like we map the DXE
page tables r/o most of the time (in the DXE page tables themselves),
regardless of various security PCDs. And the WP bit makes that mapping
effective. Is that correct?

In turn, are the DXE page tables mapped r/w in the SMM page tables?
Because, if they are, then I think CR0.WP makes no difference in SMM,
for accessing the DXE page tables. In that case, I agree we had better
not touch CR0 when the CpuDxe code executes in SMM.

> In CpuDxe, we should
> not touch SMM page table if in SMM mode. Although it looks like there's no
> problem to set this bit anyway because we just want to access DXE page table,
> I don't want to risk to expose any potential issues here. So I think it may be safer
> not to touch any SMM paging settings from non-SMM code.

OK -- please add a *lot* of comments to the code about this. :)

Thanks!
Laszlo

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Wang, Jian J]

Hi Laszlo,

Regards,
Jian


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Tuesday, June 12, 2018 4:05 PM
> To: Wang, Jian J <jian.j.wang@intel.com>; edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Dong,
> Eric <eric.dong@intel.com>; Zeng, Star <star.zeng@intel.com>
> Subject: Re: [edk2] [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE)
> page table in SMM mode
> 
> On 06/12/18 06:32, Wang, Jian J wrote:
> > Hi Laszlo,
> >
> > Thank you very much for such thorough review. I'd like to explain a bit in
> advance.
> >
> > Putting aside the specific coding issues in my patch, one thing is clear that
> SMM mode
> > has its own page table. CpuDxe should not touch it even if its public interface is
> called
> > via gBS services, because it has no knowledge of SMM internal things and it
> might
> > jeopardize SMM operation if it does. But current CpuDxe doesn't take this into
> account.
> 
> Can you explain this in more detail? I mean, when CpuDxe protocol
> members are called outside of SMM, CpuDxe obviously can't mess with SMM
> page tables, because those are located in SMRAM, and non-SMM code cannot
> access SMRAM (at that point).
> 
> So, I think you mean that, CpuDxe currently modifies whatever page
> tables are pointed-to by CR3, even if CpuDxe protocol members are
> (indirectly) invoked from code that runs in SMM. This results in CpuDxe
> messing with SMM page tables that were not meant for CpuDxe to modify.
> Is my understanding correct?
> 

Yes. I was just talking about SMM mode. CpuDxe cannot touch SMM page table
in non-SMM mode.

> > I think it should be fixed anyway, even there's no Heap Guard feature,
> 
> OK, if my above summary is correct, then this makes sense to me.
> 

So we're one the same page.

> > since current
> > design allows SMM to touch memory owned by DXE but not vice versa.
> 
> Right; it is valid for a traditional SMM driver to allocate and free
> both SMRAM and DXE memory in its entry point function.
> 
> > The basic idea of this patch is, if we want to access DXE page table in SMM
> mode, we
> > cannot do this via CR3 register (it has been reloaded by SMM) directly but via a
> stored
> > value in a global. This is feasible because page table is just a chunk of normal
> memory.
> > All we need is an entry address pointer. When exiting SMM mode back to DXE,
> the
> > updated page table will take effect immediately once CR3 is restored to DXE
> version.
> > That means we can change DXE page table attributes even in SMM mode.
> 
> Makes sense.
> 
> > Please see my other responses below.
> 
> OK:
> 
> [snip]
> 
> >> (2) Can you remind me how the SMM page tables map non-SMRAM memory?
> >>
> >> Because, I assume that, after a FreePages() libary API call, the freed
> >> memory should not be accessible to either SMM code or normal DXE code.
> >> This seems to imply that both sets of page tables should be modified.
> >> What am I missing?
> >>
> >
> > For both SMM and DXE, we just do 1:1 full memory mapping in paging but in
> > different page tables. As far as I know, there's no such rules that freed pages
> > should not be accessible. I think it's just implementation dependent.
> 
> Let's assume we have a DXE page allocation, with the heap guard enabled.
> To my understanding, the allocated area is preceded and succeeded by 1
> page on each end, and both of those pages are marked as inaccessible, so
> that we get a page fault on buffer overflow / underflow.
> 
> Because traditional SMM drivers are allowed to work with DXE memory in
> their entry point functions, I *believe* (I'm not sure) that the guard
> pages should be marked as inaccessible in both the DXE page tables and
> in the SMM page tables; so that buffer overflow/underflow trap in both
> operating modes. Is my understanding correct?
> 
> FWIW, I based my question in (2) on the above understading -- if the
> heap guard feature marks the guard pages inaccessible for both operating
> modes at allocation time, then whatever PTE-level "undo" is performed at
> gBS->FreePages() time, should likely also modify both sets of page tables.
> 
> Of course, if the DXE heap guard modifies the PTEs for the guard pages
> only in the DXE page tables, at allocation time, then the SMM page
> tables are irrelevant at release time as well.
> 

You're right from this perspective. But it means we need to merge DXE page table
into SMM page table during switching into SMM mode and sync back when exiting.
We need to be very careful to make sure SMRAM part won't be changed inexpertly.
Considering there's few chances we'll free DXE memory in SMM mode, I'm not sure
this is worthy of doing in practice, not to mention Heap Guard is just a debug feature.
Furthermore, we do a lot of security things in SMM mode. I'm wondering if such 
kind of change will compromise any security scheme. Maybe we need more study
on this.

> [snip]
> 
> >>> +BOOLEAN
> >>> +IsInSmm (
> >>> +  VOID
> >>> +  )
> >>> +{
> >>> +  EFI_STATUS              Status;
> >>> +  BOOLEAN                 InSmm;
> >>> +
> >>> +  InSmm = FALSE;
> >>> +  if (mSmmBase2 == NULL) {
> >>> +    Status = gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL,
> >>> +                                  (VOID **)&mSmmBase2);
> >>
> >> (7) Indentation is incorrect.
> >>
> >
> > It'll be fixed.
> >
> >>> +    if (EFI_ERROR (Status)) {
> >>> +      mSmmBase2 = NULL;
> >>> +    }
> >>
> >> (8) I think you can simply drop the EFI_ERROR (Status) branch;
> >> "mSmmBase2" will simply remain NULL.
> 
> You didn't comment on this: do you agree, or is it necessary for some
> reason to re-assign NULL to mSmmBase2? (For example, a static analyzer
> or a compiler complains etc...)
> 

Sorry I missed this one. I agree with you. I think there should no static analyzer
issue here.

> [snip]
> 
> >>> @@ -507,7 +537,10 @@ IsReadOnlyPageWriteProtected (
> >>>    VOID
> >>>    )
> >>>  {
> >>> -  return ((AsmReadCr0 () & BIT16) != 0);
> >>> +  if (!IsInSmm ()) {
> >>> +    return ((AsmReadCr0 () & BIT16) != 0);
> >>> +  }
> >>> +  return FALSE;
> >>>  }
> >>>
> >>>  /**
> >>> @@ -518,7 +551,9 @@ DisableReadOnlyPageWriteProtect (
> >>>    VOID
> >>>    )
> >>>  {
> >>> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> >>> +  if (!IsInSmm ()) {
> >>> +    AsmWriteCr0 (AsmReadCr0 () & ~BIT16);
> >>> +  }
> >>>  }
> >>>
> >>>  /**
> >>> @@ -529,7 +564,9 @@ EnableReadOnlyPageWriteProtect (
> >>>    VOID
> >>>    )
> >>>  {
> >>> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> >>> +  if (!IsInSmm ()) {
> >>> +    AsmWriteCr0 (AsmReadCr0 () | BIT16);
> >>> +  }
> >>>  }
> >>
> >> (11) I don't understand these changes at all. Why are these functions
> >> no-ops when CpuDxe is invoked in Management Mode?
> >>
> >
> > CR0.BIT16 is used to protect current (active) page table.
> 
> The SDM writes,
> 
>     Write Protect (bit 16 of CR0) — When set, inhibits supervisor-level
>     procedures from writing into read-only pages; when clear, allows
>     supervisor-level procedures to write into read-only pages
>     (regardless of the U/S bit setting; see Section 4.1.3 and Section
>     4.6). This flag facilitates implementation of the copy-on-write
>     method of creating a new process (forking) used by operating systems
>     such as UNIX.
> 
> First, I think we should use a macro for "write protect" that is more
> telling than just BIT16. We have "CR0_WP" in both
> - MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h
> - UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
> 
> so maybe we should move that to some IndustryStandard header. Should be
> a separate patch / BZ anyway.
> 
> Second, from looking at the CpuDxe code, it seems like we map the DXE
> page tables r/o most of the time (in the DXE page tables themselves),
> regardless of various security PCDs. And the WP bit makes that mapping
> effective. Is that correct?
> 
> In turn, are the DXE page tables mapped r/w in the SMM page tables?
> Because, if they are, then I think CR0.WP makes no difference in SMM,
> for accessing the DXE page tables. In that case, I agree we had better
> not touch CR0 when the CpuDxe code executes in SMM.
> 

I agree using CR0_WP is better. I'll change it in v2.
And SMM page tables doesn't take care of DXE page tables. So in SMM mode,
DXE page tables are not protected. But still, it'd be better not to touch SMM
paging settings in non-SMM code.

> > In CpuDxe, we should
> > not touch SMM page table if in SMM mode. Although it looks like there's no
> > problem to set this bit anyway because we just want to access DXE page table,
> > I don't want to risk to expose any potential issues here. So I think it may be
> safer
> > not to touch any SMM paging settings from non-SMM code.
> 
> OK -- please add a *lot* of comments to the code about this. :)
> 

Sure.

> Thanks!
> Laszlo

Re: [PATCH 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Laszlo Ersek]

On 06/12/18 10:44, Wang, Jian J wrote:
> Hi Laszlo,

thanks for the answers, they sound OK to me. From my side, please feel
free to post v2.

Thanks!
Laszlo

[PATCH 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection

[Jian J Wang]

CpuDxe driver is updated to be able to access DXE page table in SMM mode,
which means Heap Guard can get correct memory paging attributes in what
ever modes. It's not necessary to exclude SMM from detecting Heap Guard
feature support.

Change-Id: I5310e6e49a258ac7a9240e40c8c99cdb692c1e02
Cc: Star Zeng <star.zeng@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Ruiyu Ni <ruiyu.ni@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
---
 MdeModulePkg/Core/Dxe/Mem/HeapGuard.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
index 9d765c98f6..447c56bb11 100644
--- a/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
+++ b/MdeModulePkg/Core/Dxe/Mem/HeapGuard.c
@@ -667,21 +667,11 @@ IsMemoryTypeToGuard (
 {
   UINT64 TestBit;
   UINT64 ConfigBit;
-  BOOLEAN     InSmm;
 
   if (AllocateType == AllocateAddress) {
     return FALSE;
   }
 
-  InSmm = FALSE;
-  if (gSmmBase2 != NULL) {
-    gSmmBase2->InSmm (gSmmBase2, &InSmm);
-  }
-
-  if (InSmm) {
-    return FALSE;
-  }
-
   if ((PcdGet8 (PcdHeapGuardPropertyMask) & PageOrPool) == 0) {
     return FALSE;
   }
-- 
2.16.2.windows.1