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

[Andrew Fish <afish@apple.com>]

> On Jun 12, 2018, at 10:35 PM, Jian J Wang <jian.j.wang@intel.com> 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.
> 

Are there any security implications having SMM depend on attacker controlled data (DXE page tables)?

Thanks,

Andrew Fish

> 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
> 
> [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
> +
> ///
> /// 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) {
> +      AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, (UINT32 *)&RegEdx);
> +
> +      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);
> +  }
> }
> 
> /**
> -- 
> 2.16.2.windows.1
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel