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

["Dong, Eric" <eric.dong@intel.com>]

Reviewed-by: Eric Dong <eric.dong@intel.com>

-----Original Message-----
From: Wang, Jian J 
Sent: Thursday, June 14, 2018 8:47 AM
To: Laszlo Ersek <lersek@redhat.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 v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

Hi Laszlo,

Regards,
Jian

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, June 13, 2018 11:10 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 v2 1/2] UefiCpuPkg/CpuDxe: allow accessing 
> (DXE) page table in SMM mode
> 
> 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.
> 

Make sense.

> >
> >  [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.
> 

Sure.

> > +
> >  ///
> >  /// 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 
> > + gBS->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_SUPPO
> RT;
> > +
> > +    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.
> 


Ok.

> > +      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.
> 

Your way is better. At least it saves a type cast.

> 
> 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.
> 

Thank you very much for the test.

> 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_SUPPO
> RT;
> > +      }
> >      }
> >    }
> > +
> > +  //
> > +  // 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);  }
> >  }
> >
> >  /**
> >