* [PATCH v2 0/2] fix DXE memory free issue in SMM mode
@ 2018-06-13 5:34 Jian J Wang
2018-06-13 5:35 ` [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table " Jian J Wang
2018-06-13 5:35 ` [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection Jian J Wang
0 siblings, 2 replies; 9+ messages in thread
From: Jian J Wang @ 2018-06-13 5:34 UTC (permalink / raw)
To: edk2-devel
> v2:
> a. add more specific explanations in commit message
> b. add more comments in code
> c. remove redundant logic in code
> d. fix logic hole in code
> e. replace meanless constant macro with meaning ones
> f. remove irrelated changes
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.inf | 1 +
UefiCpuPkg/CpuDxe/CpuPageTable.c | 159 ++++++++++++++++++++++++++--------
3 files changed, 123 insertions(+), 47 deletions(-)
--
2.16.2.windows.1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-13 5:34 [PATCH v2 0/2] fix DXE memory free issue in SMM mode Jian J Wang @ 2018-06-13 5:35 ` Jian J Wang 2018-06-13 15:10 ` Laszlo Ersek 2018-06-13 15:14 ` Andrew Fish 2018-06-13 5:35 ` [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection Jian J Wang 1 sibling, 2 replies; 9+ messages in thread From: Jian J Wang @ 2018-06-13 5:35 UTC (permalink / raw) To: edk2-devel; +Cc: Eric Dong, Laszlo Ersek, Jiewen Yao, Ruiyu Ni > 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 [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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-13 5:35 ` [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table " Jian J Wang @ 2018-06-13 15:10 ` Laszlo Ersek 2018-06-14 0:46 ` Wang, Jian J 2018-06-13 15:14 ` Andrew Fish 1 sibling, 1 reply; 9+ messages in thread From: Laszlo Ersek @ 2018-06-13 15:10 UTC (permalink / raw) To: Jian J Wang, edk2-devel; +Cc: Ruiyu Ni, Jiewen Yao, Eric Dong 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); > + } > } > > /** > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-13 15:10 ` Laszlo Ersek @ 2018-06-14 0:46 ` Wang, Jian J 2018-06-14 2:01 ` Dong, Eric 0 siblings, 1 reply; 9+ messages in thread From: Wang, Jian J @ 2018-06-14 0:46 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Yao, Jiewen, Dong, Eric 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 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); > > + } > > } > > > > /** > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-14 0:46 ` Wang, Jian J @ 2018-06-14 2:01 ` Dong, Eric 0 siblings, 0 replies; 9+ messages in thread From: Dong, Eric @ 2018-06-14 2:01 UTC (permalink / raw) To: Wang, Jian J, Laszlo Ersek, edk2-devel@lists.01.org Cc: Ni, Ruiyu, Yao, Jiewen 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); } > > } > > > > /** > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-13 5:35 ` [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table " Jian J Wang 2018-06-13 15:10 ` Laszlo Ersek @ 2018-06-13 15:14 ` Andrew Fish 2018-06-13 19:54 ` Laszlo Ersek 1 sibling, 1 reply; 9+ messages in thread From: Andrew Fish @ 2018-06-13 15:14 UTC (permalink / raw) To: Jian J Wang; +Cc: edk2-devel, Ruiyu Ni, Jiewen Yao, Laszlo Ersek, Eric Dong > 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode 2018-06-13 15:14 ` Andrew Fish @ 2018-06-13 19:54 ` Laszlo Ersek 0 siblings, 0 replies; 9+ messages in thread From: Laszlo Ersek @ 2018-06-13 19:54 UTC (permalink / raw) To: Andrew Fish, Jian J Wang; +Cc: edk2-devel, Ruiyu Ni, Jiewen Yao, Eric Dong On 06/13/18 17:14, Andrew Fish wrote: > > >> 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)? This thought crossed my mind as well, and a part of the questions that I raised earlier are related. The important thing to consider here is the call chain; in other words, the circumstances under which we can reach CpuDxe code *at all* while the processor is in SMM. The call chain starts when an SMM driver (a traditional SMM driver I reckon) calls FreePool() or FreePages(), and the SmmMemoryAllocationLib instance determines that the SMM driver is freeing *DXE* pool (or pages) memory, and not SMRAM. In this case, SmmMemoryAllocationLib forwards the call out to gBS->FreePool() or gBS->FreePages(). And, it is *from there* that we gradually reach CpuDxe (the provider of the CpuArch protocol) to mess with DXE page table entries -- because the gBS->FreePool() and gBS->FreePages() services do that in edk2, regardless of SMM, when the heap guard is enabled. Obviously, gBS points into attacker controlled memory, so as soon as SmmMemoryAllocationLib calls gBS->anything, the calling SMM driver has been compromised immediately, way before we reach CpuDxe or anything else in DXE. So why isn't SmmMemoryAllocationLib already broken? Because, I think, a traditional SMM driver *must not* call FreePool() and FreePages(), for potentially releasing DXE memory, outside its entry point function anyway. That is, such FreePool / FreePages calls are permitted only under the exact same circumstances where the driver is allowed to utilize any other DXE protocols and services anyway; despite potentially executing in SMM. Once all DXE and SMM drivers have been dispatched and we reach BDS, SMRAM is locked down (by platform BDS), and SMM drivers aren't allowed to touch *anything* DXE any longer. In other words, it is safe to rely on CpuDxe global variables in SMM because we only do that when calling gBS->anything is still safe as well -- that is, before any 3rd party code gets a chance to run. In my previous review at http://mid.mail-archive.com/aada511c-bdb9-d833-caa5-bee56cc47d27@redhat.com this topic was the subject of my question (9a): > 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() I didn't get a clear answer to that, but I managed to convince myself, with the above argument. If any of the new code added to CpuDxe is reached in SMM *after* 3rd party code was launched, then at least one SMM driver messed up (calling FreePool / FreePages on DXE memory at the wrong time), and then the vulnerability exists in that driver without the CpuDxe changes already. Thus, I believe this series does not introduce a vulnerability or new risks, it just adds more logic to the end of a call chain that is *already* - either broken (if an SMM driver calls FreePool / FreePages on DXE memory at the wrong time), - or safe (if all SMM drivers call FreePool / FreePages on DXE memory only in their entry point functions). More generally, to be honest: I find it terribly risky that SmmMemoryAllocationLib contains convenience code like this. In my opinion, SmmMemoryAllocationLib should never ever silently call out to gBS->anything; if it determines that the memory to release is outside of SMRAM, it should call CpuDeadLoop(). SMM drivers that want to release DXE memory should always call gBS services directly, not through MemoryAllocationLib APIs. Such a clear distinction would have also made the verification of this patch a lot easier -- the argument would have started with, "CpuDxe code is reached in SMM when SMM drivers call gBS->FreeXxx()". And the restrictions on gBS->FreeXxx() (namely, that they are restricted to the entry point function) make for a simpler argument. Thanks, Laszlo ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection 2018-06-13 5:34 [PATCH v2 0/2] fix DXE memory free issue in SMM mode Jian J Wang 2018-06-13 5:35 ` [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table " Jian J Wang @ 2018-06-13 5:35 ` Jian J Wang 2018-06-14 0:58 ` Zeng, Star 1 sibling, 1 reply; 9+ messages in thread From: Jian J Wang @ 2018-06-13 5:35 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Eric Dong, Jiewen Yao, Ruiyu Ni 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 environment. It's not necessary to exclude SMM from detecting Heap Guard feature support. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection 2018-06-13 5:35 ` [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection Jian J Wang @ 2018-06-14 0:58 ` Zeng, Star 0 siblings, 0 replies; 9+ messages in thread From: Zeng, Star @ 2018-06-14 0:58 UTC (permalink / raw) To: Wang, Jian J, edk2-devel@lists.01.org Cc: Dong, Eric, Yao, Jiewen, Ni, Ruiyu, Zeng, Star Reviewed-by: Star Zeng <star.zeng@intel.com> -----Original Message----- From: Wang, Jian J Sent: Wednesday, June 13, 2018 1:35 PM To: edk2-devel@lists.01.org Cc: Zeng, Star <star.zeng@intel.com>; Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com> Subject: [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection 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 environment. It's not necessary to exclude SMM from detecting Heap Guard feature support. 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-06-14 2:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-06-13 5:34 [PATCH v2 0/2] fix DXE memory free issue in SMM mode Jian J Wang 2018-06-13 5:35 ` [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table " Jian J Wang 2018-06-13 15:10 ` Laszlo Ersek 2018-06-14 0:46 ` Wang, Jian J 2018-06-14 2:01 ` Dong, Eric 2018-06-13 15:14 ` Andrew Fish 2018-06-13 19:54 ` Laszlo Ersek 2018-06-13 5:35 ` [PATCH v2 2/2] MdeModulePkg/Core: remove SMM check for Heap Guard feature detection Jian J Wang 2018-06-14 0:58 ` Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox