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