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

[Laszlo Ersek <lersek@redhat.com>]

Hi Jian,

I have three comments:

On 06/13/18 07:35, Jian J Wang wrote:
>> v2:
>>   a. add more specific explanations in commit message
>>   b. add more comments in code
>>   c. remove redundant logic in IsInSmm()
>>   d. fix a logic hole in GetCurrentPagingContext()
>>   e. replace meanless constant macro with meaning ones
> 
> The MdePkg/Library/SmmMemoryAllocationLib, used only by DXE_SMM_DRIVER,
> 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. If some memory related
> features, like Heap Guard, are enabled, gBS interface will turn to
> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), provided by
> DXE driver UefiCpuPkg/CpuDxe, to change memory paging attributes. This
> means we have part of DXE code running in SMM mode in certain
> circumstances.
> 
> Because page table in SMM mode is different from DXE mode and CpuDxe
> always uses current registers (CR0, CR3, etc.) to get memory paging
> attributes, it cannot get the correct attributes of DXE memory in SMM
> mode from SMM page table. This will cause incorrect memory manipulations,
> like fail the releasing of Guard pages if Heap Guard is enabled.
> 
> 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.
> This can avoid retrieving wrong DXE memory paging attributes and changing
> SMM page table attributes unexpectedly.
> 
> 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.inf     |   1 +
>  UefiCpuPkg/CpuDxe/CpuPageTable.c | 159 ++++++++++++++++++++++++++++++---------
>  2 files changed, 123 insertions(+), 37 deletions(-)
> 
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> index 3c938cee53..ce2bd3627c 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -66,6 +66,7 @@
>  [Protocols]
>    gEfiCpuArchProtocolGuid                       ## PRODUCES
>    gEfiMpServiceProtocolGuid                     ## PRODUCES
> +  gEfiSmmBase2ProtocolGuid                      ## CONSUMES

(1) In my v1 review, I suggested "SOMETIMES_CONSUMES" for this hint.

And that's because CpuDxe can be included in platform builds that don't
include the SMM driver stack at all; in that case,
gEfiSmmBase2ProtocolGuid will not be available and will not be consumed.
In other words, we cannot say that the protocol is always consumed.

>  
>  [Guids]
>    gIdleLoopEventGuid                            ## CONSUMES           ## Event
> diff --git a/UefiCpuPkg/CpuDxe/CpuPageTable.c b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> index e2595b4d89..b7e75922b6 100644
> --- a/UefiCpuPkg/CpuDxe/CpuPageTable.c
> +++ b/UefiCpuPkg/CpuDxe/CpuPageTable.c
> @@ -23,10 +23,21 @@
>  #include <Library/DebugLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  #include <Protocol/MpService.h>
> +#include <Protocol/SmmBase2.h>
> +#include <Register/Cpuid.h>
> +#include <Register/Msr.h>
>  
>  #include "CpuDxe.h"
>  #include "CpuPageTable.h"
>  
> +///
> +/// Paging registers
> +///
> +#define CR0_WP                      BIT16
> +#define CR0_PG                      BIT31
> +#define CR4_PSE                     BIT4
> +#define CR4_PAE                     BIT5

(2a) I think the BITxx -> CRx_xx macro replacements should have been
split to a separate patch.

> +
>  ///
>  /// Page Table Entry
>  ///
> @@ -87,7 +98,46 @@ 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;
> +
> +/**
> + Check if current execution environment is in SMM mode or not, via
> + EFI_SMM_BASE2_PROTOCOL.
> +
> + This is necessary because of the fact that MdePkg\Library\SmmMemoryAllocationLib
> + supports to free memory outside SMRAM. The library will call gBS->FreePool() or
> + gBS->FreePages() and then SetMemorySpaceAttributes interface in turn to change
> + memory paging attributes during free operation, if some memory related features
> + are enabled (like Heap Guard).
> +
> + This means that SetMemorySpaceAttributes() has chance to run in SMM mode. This
> + will cause incorrect result because SMM mode always loads its own page tables,
> + which are usually different from DXE. This function can be used to detect such
> + situation and help to avoid further misoperations.
> +
> +  @retval TRUE    In SMM mode.
> +  @retval FALSE   Not in SMM mode.
> +**/
> +BOOLEAN
> +IsInSmm (
> +  VOID
> +  )
> +{
> +  BOOLEAN                 InSmm;
> +
> +  InSmm = FALSE;
> +  if (mSmmBase2 == NULL) {
> +    gBS->LocateProtocol (&gEfiSmmBase2ProtocolGuid, NULL, (VOID **)&mSmmBase2);
> +  }
> +
> +  if (mSmmBase2 != NULL) {
> +    mSmmBase2->InSmm (mSmmBase2, &InSmm);
> +  }
> +
> +  return InSmm;
> +}
>  
>  /**
>    Return current paging context.
> @@ -99,45 +149,61 @@ GetCurrentPagingContext (
>    IN OUT PAGE_TABLE_LIB_PAGING_CONTEXT     *PagingContext
>    )
>  {
> -  UINT32                         RegEax;
> -  UINT32                         RegEdx;
> +  UINT32                          RegEax;
> +  CPUID_EXTENDED_CPU_SIG_EDX      RegEdx;
> +  MSR_IA32_EFER_REGISTER          MsrEfer;
>  
> -  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;
> -  }
> +  //
> +  // Don't retrieve current paging context from processor if in SMM mode.
> +  //
> +  if (!IsInSmm ()) {
> +    ZeroMem (&mPagingContext, sizeof(mPagingContext));
>  
> -  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 (sizeof(UINTN) == sizeof(UINT64)) {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_X64;
> +    } else {
> +      mPagingContext.MachineType = IMAGE_FILE_MACHINE_I386;
> +    }
> +    if ((AsmReadCr0 () & CR0_PG) != 0) {
> +      mPagingContext.ContextData.X64.PageTableBase = (AsmReadCr3 () & PAGING_4K_ADDRESS_MASK_64);
> +    } else {
> +      mPagingContext.ContextData.X64.PageTableBase = 0;
> +    }
>  
> -  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;
> -      }
> +    if ((AsmReadCr4 () & CR4_PSE) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PSE;
> +    }
> +    if ((AsmReadCr4 () & CR4_PAE) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAE;
> +    }
> +    if ((AsmReadCr0 () & CR0_WP) != 0) {
> +      mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_WP_ENABLE;
>      }
> -    if ((RegEdx & BIT26) != 0) {
> -      PagingContext->ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
> +
> +    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
> +    if (RegEax >= CPUID_EXTENDED_CPU_SIG) {

(2b) Similarly to (2a), this cleanup (which is correct, and welcome)
should have been split to a separate patch.

> +      AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, (UINT32 *)&RegEdx);

(3) I think it would be more idiomatic to pass &RegEdx.Uint32 as the
last argument. However, this method works as well.


I don't think that posting a v3 is necessary just because of these
remarks. Please fix (1) though, just before you push the patch.

* For this patch, with (1) updated:

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

* For the series:

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

For the regression testing, I used OVMF built both with and without SMM.
Also, for patch #2, please remember that OVMF does not enable the page
guard feature.

Thanks!
Laszlo


> +
> +      if (RegEdx.Bits.NX != 0) {
> +        // XD supported
> +        MsrEfer.Uint64 = AsmReadMsr64(MSR_CORE_IA32_EFER);
> +        if (MsrEfer.Bits.NXE != 0) {
> +          // XD activated
> +          mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_XD_ACTIVATED;
> +        }
> +      }
> +
> +      if (RegEdx.Bits.Page1GB != 0) {
> +        mPagingContext.ContextData.Ia32.Attributes |= PAGE_TABLE_LIB_PAGING_CONTEXT_IA32_X64_ATTRIBUTES_PAGE_1G_SUPPORT;
> +      }
>      }
>    }
> +
> +  //
> +  // This can avoid getting SMM paging context if in SMM mode. We cannot assume
> +  // SMM mode shares the same paging context as DXE.
> +  //
> +  CopyMem (PagingContext, &mPagingContext, sizeof (mPagingContext));
>  }
>  
>  /**
> @@ -507,7 +573,14 @@ IsReadOnlyPageWriteProtected (
>    VOID
>    )
>  {
> -  return ((AsmReadCr0 () & BIT16) != 0);
> +  //
> +  // To avoid unforseen consequences, don't touch paging settings in SMM mode
> +  // in this driver.
> +  //
> +  if (!IsInSmm ()) {
> +    return ((AsmReadCr0 () & CR0_WP) != 0);
> +  }
> +  return FALSE;
>  }
>  
>  /**
> @@ -518,7 +591,13 @@ DisableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() & ~BIT16);
> +  //
> +  // To avoid unforseen consequences, don't touch paging settings in SMM mode
> +  // in this driver.
> +  //
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () & ~CR0_WP);
> +  }
>  }
>  
>  /**
> @@ -529,7 +608,13 @@ EnableReadOnlyPageWriteProtect (
>    VOID
>    )
>  {
> -  AsmWriteCr0 (AsmReadCr0() | BIT16);
> +  //
> +  // To avoid unforseen consequences, don't touch paging settings in SMM mode
> +  // in this driver.
> +  //
> +  if (!IsInSmm ()) {
> +    AsmWriteCr0 (AsmReadCr0 () | CR0_WP);
> +  }
>  }
>  
>  /**
>