Re: [edk2] [PATCH V2] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.

[Fan, Jeff]

Reviewed-by: Jeff Fan 

-Original Message-
From: Yao, Jiewen 
Sent: Thursday, December 01, 2016 8:04 PM
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek; Fan, Jeff; Kinney, Michael D
Subject: [PATCH V2] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.

This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=246

Previously, when SMM exception happens after EndOfDxe, with StackGuard enabled 
on IA32, the #double fault exception is reported instead of #page fault.

Root cause is below:

Current EDKII SMM page protection will lock GDT.
If IA32 stack guard is enabled, the page fault handler will do task switch.
This task switch need write busy flag in GDT, and write TSS.

However, the GDT and TSS is locked at that time, so the double fault happens.

We decide to not lock GDT for IA32 StackGuard enabled.

This issue does not exist on X64, or IA32 without StackGuard.

Cc: Laszlo Ersek 
Cc: Jeff Fan 
Cc: Michael D Kinney 
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao 
---
 UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c  | 55 
 UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 68 
 UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 --
 UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c   | 49 +-
 4 files changed, 171 insertions(+), 49 deletions(-)

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c 
b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
index f4db6c8..3c68c97 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
@@ -127,6 +127,61 @@ InitGdt (
 }
 
 /**
+  This function sets GDT/IDT buffer to be RO and XP.
+**/
+VOID
+PatchGdtIdtMap (
+  VOID
+  )
+{
+  EFI_PHYSICAL_ADDRESS   BaseAddress;
+  UINTN  Size;
+
+  //
+  // GDT
+  //
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));
+
+  BaseAddress = mGdtBuffer;
+  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);  if (!FeaturePcdGet 
+ (PcdCpuSmmStackGuard)) {
+//
+// Do not set RO for IA32 when stack guard feature is enabled.
+// Stack Guard need use task switch to switch stack.
+// It need write GDT and TSS.
+//
+SmmSetMemoryAttributes (
+  BaseAddress,
+  Size,
+  EFI_MEMORY_RO
+  );
+  }
+  SmmSetMemoryAttributes (
+BaseAddress,
+Size,
+EFI_MEMORY_XP
+);
+
+  //
+  // IDT
+  //
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));
+
+  BaseAddress = gcSmiIdtr.Base;
+  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);
+  SmmSetMemoryAttributes (
+BaseAddress,
+Size,
+EFI_MEMORY_RO
+);
+  SmmSetMemoryAttributes (
+BaseAddress,
+Size,
+EFI_MEMORY_XP
+);
+}
+
+/**
   Transfer AP to safe hlt-loop after it finished restore CPU features on S3 
patch.
 
   @param[in] ApHltLoopCode  The address of the safe hlt-loop function.
diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h 
b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index abe5cc6..c415858 100644
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -524,6 +524,14 @@ InitGdt (
   );
 
 /**
+  This function sets GDT/IDT buffer to be RO and XP.
+**/
+VOID
+PatchGdtIdtMap (
+  VOID
+  );
+
+/**
 
   Register the SMM Foundation entry point.
 
@@ -596,6 +604,66 @@ SmmBlockingStartupThisAp (
   );
 
 /**
+  This function sets the attributes for the memory region specified by 
+ BaseAddress and  Length from their current attributes to the attributes 
specified by Attributes.
+
+  @param[in]  BaseAddress  The physical address that is the start address 
of a memory region.
+  @param[in]  Length   The size in bytes of the memory region.
+  @param[in]  Attributes   The bit mask of attributes to set for the 
memory region.
+
+  @retval EFI_SUCCESS   The attributes were set for the memory region.
+  @retval EFI_ACCESS_DENIED The attributes for the memory resource range 
specified by
+BaseAddress and Length cannot be modified.
+  @retval EFI_INVALID_PARAMETER Length is zero.
+Attributes specified an illegal combination of 
attributes that
+cannot be set together.
+  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to 
modify the attributes of
+the memory resource range.
+  @retval EFI_UNSUPPORTED   The processor does not support one or more 
bytes of the memory
+resource range specified by BaseAddress and 
Length.
+The bit mask of attributes is not support for 
the memory resource
+range specified by BaseAddress and Length.
+
+**/
+EFI_STATUS
+EFIAPI
+SmmSetMemoryAttributes (
+  IN  

Re: [edk2] [PATCH V2] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.

[Laszlo Ersek]

On 12/01/16 13:04, Jiewen Yao wrote:
> This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=246
> 
> Previously, when SMM exception happens after EndOfDxe,
> with StackGuard enabled on IA32, the #double fault exception
> is reported instead of #page fault.
> 
> Root cause is below:
> 
> Current EDKII SMM page protection will lock GDT.
> If IA32 stack guard is enabled, the page fault handler will do task switch.
> This task switch need write busy flag in GDT, and write TSS.
> 
> However, the GDT and TSS is locked at that time, so the
> double fault happens.
> 
> We decide to not lock GDT for IA32 StackGuard enabled.
> 
> This issue does not exist on X64, or IA32 without StackGuard.
> 
> Cc: Laszlo Ersek 
> Cc: Jeff Fan 
> Cc: Michael D Kinney 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Jiewen Yao 
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c  | 55 
>  UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 68 
>  UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 --
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c   | 49 +-
>  4 files changed, 171 insertions(+), 49 deletions(-)

Regression-tested-by: Laszlo Ersek 

___
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel