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 <lersek@redhat.com>
Cc: Jeff Fan <jeff.fan@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Jiewen Yao <jiewen.yao@intel.com>
Reviewed-by: Jeff Fan <jeff.fan@intel.com>
Regression-tested-by: Laszlo Ersek <lersek@redhat.com>

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
index f4db6c8b60cc0f7d6b0e2de54d85a6fab4ae8fbd..3c68c970245fba7369dde77e2adefac026f58b09 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c
@@ -126,6 +126,61 @@ InitGdt (
   return GdtTssTables;\r
 }\r
 \r
+/**\r
+  This function sets GDT/IDT buffer to be RO and XP.\r
+**/\r
+VOID\r
+PatchGdtIdtMap (\r
+  VOID\r
+  )\r
+{\r
+  EFI_PHYSICAL_ADDRESS       BaseAddress;\r
+  UINTN                      Size;\r
+\r
+  //\r
+  // GDT\r
+  //\r
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));\r
+\r
+  BaseAddress = mGdtBuffer;\r
+  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);\r
+  if (!FeaturePcdGet (PcdCpuSmmStackGuard)) {\r
+    //\r
+    // Do not set RO for IA32 when stack guard feature is enabled.\r
+    // Stack Guard need use task switch to switch stack.\r
+    // It need write GDT and TSS.\r
+    //\r
+    SmmSetMemoryAttributes (\r
+      BaseAddress,\r
+      Size,\r
+      EFI_MEMORY_RO\r
+      );\r
+  }\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_XP\r
+    );\r
+\r
+  //\r
+  // IDT\r
+  //\r
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));\r
+\r
+  BaseAddress = gcSmiIdtr.Base;\r
+  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_RO\r
+    );\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_XP\r
+    );\r
+}\r
+\r
 /**\r
   Transfer AP to safe hlt-loop after it finished restore CPU features on S3 patch.\r
 \r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
index 611cbc190a1dd0826e16e985817aa19462d43f5a..9160fa8b8a579d2b059970280be57c019e8582a9 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h
@@ -497,6 +497,14 @@ InitGdt (
   OUT UINTN  *GdtStepSize\r
   );\r
 \r
+/**\r
+  This function sets GDT/IDT buffer to be RO and XP.\r
+**/\r
+VOID\r
+PatchGdtIdtMap (\r
+  VOID\r
+  );\r
+\r
 /**\r
 \r
   Register the SMM Foundation entry point.\r
@@ -569,6 +577,66 @@ SmmBlockingStartupThisAp (
   IN OUT  VOID                      *ProcArguments OPTIONAL\r
   );\r
 \r
+/**\r
+  This function sets the attributes for the memory region specified by BaseAddress and\r
+  Length from their current attributes to the attributes specified by Attributes.\r
+\r
+  @param[in]  BaseAddress      The physical address that is the start address of a memory region.\r
+  @param[in]  Length           The size in bytes of the memory region.\r
+  @param[in]  Attributes       The bit mask of attributes to set for the memory region.\r
+\r
+  @retval EFI_SUCCESS           The attributes were set for the memory region.\r
+  @retval EFI_ACCESS_DENIED     The attributes for the memory resource range specified by\r
+                                BaseAddress and Length cannot be modified.\r
+  @retval EFI_INVALID_PARAMETER Length is zero.\r
+                                Attributes specified an illegal combination of attributes that\r
+                                cannot be set together.\r
+  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to modify the attributes of\r
+                                the memory resource range.\r
+  @retval EFI_UNSUPPORTED       The processor does not support one or more bytes of the memory\r
+                                resource range specified by BaseAddress and Length.\r
+                                The bit mask of attributes is not support for the memory resource\r
+                                range specified by BaseAddress and Length.\r
+\r
+**/\r
+EFI_STATUS\r
+EFIAPI\r
+SmmSetMemoryAttributes (\r
+  IN  EFI_PHYSICAL_ADDRESS                       BaseAddress,\r
+  IN  UINT64                                     Length,\r
+  IN  UINT64                                     Attributes\r
+  );\r
+\r
+/**\r
+  This function clears the attributes for the memory region specified by BaseAddress and\r
+  Length from their current attributes to the attributes specified by Attributes.\r
+\r
+  @param[in]  BaseAddress      The physical address that is the start address of a memory region.\r
+  @param[in]  Length           The size in bytes of the memory region.\r
+  @param[in]  Attributes       The bit mask of attributes to clear for the memory region.\r
+\r
+  @retval EFI_SUCCESS           The attributes were cleared for the memory region.\r
+  @retval EFI_ACCESS_DENIED     The attributes for the memory resource range specified by\r
+                                BaseAddress and Length cannot be modified.\r
+  @retval EFI_INVALID_PARAMETER Length is zero.\r
+                                Attributes specified an illegal combination of attributes that\r
+                                cannot be set together.\r
+  @retval EFI_OUT_OF_RESOURCES  There are not enough system resources to modify the attributes of\r
+                                the memory resource range.\r
+  @retval EFI_UNSUPPORTED       The processor does not support one or more bytes of the memory\r
+                                resource range specified by BaseAddress and Length.\r
+                                The bit mask of attributes is not support for the memory resource\r
+                                range specified by BaseAddress and Length.\r
+\r
+**/\r
+EFI_STATUS\r
+EFIAPI\r
+SmmClearMemoryAttributes (\r
+  IN  EFI_PHYSICAL_ADDRESS                       BaseAddress,\r
+  IN  UINT64                                     Length,\r
+  IN  UINT64                                     Attributes\r
+  );\r
+\r
 /**\r
   Initialize MP synchronization data.\r
 \r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
index 6becc1b445264f736f2ea8097994cba3426fca5d..588aa270d895375dbc872d5a749523542a9f10e5 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c
@@ -749,54 +749,6 @@ PatchSmmSaveStateMap (
     );\r
 }\r
 \r
-/**\r
-  This function sets GDT/IDT buffer to be RO and XP.\r
-**/\r
-VOID\r
-PatchGdtIdtMap (\r
-  VOID\r
-  )\r
-{\r
-  EFI_PHYSICAL_ADDRESS       BaseAddress;\r
-  UINTN                      Size;\r
-\r
-  //\r
-  // GDT\r
-  //\r
-  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));\r
-\r
-  BaseAddress = mGdtBuffer;\r
-  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);\r
-  SmmSetMemoryAttributes (\r
-    BaseAddress,\r
-    Size,\r
-    EFI_MEMORY_RO\r
-    );\r
-  SmmSetMemoryAttributes (\r
-    BaseAddress,\r
-    Size,\r
-    EFI_MEMORY_XP\r
-    );\r
-\r
-  //\r
-  // IDT\r
-  //\r
-  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));\r
-\r
-  BaseAddress = gcSmiIdtr.Base;\r
-  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);\r
-  SmmSetMemoryAttributes (\r
-    BaseAddress,\r
-    Size,\r
-    EFI_MEMORY_RO\r
-    );\r
-  SmmSetMemoryAttributes (\r
-    BaseAddress,\r
-    Size,\r
-    EFI_MEMORY_XP\r
-    );\r
-}\r
-\r
 /**\r
   This function sets memory attribute according to MemoryAttributesTable.\r
 **/\r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
index 9fc00c1578c1f1c57fe0fd8bb6d89a60167906a4..9d26e44a9acd58c1f279694d0afa3eac2498a544 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
@@ -95,6 +95,54 @@ InitGdt (
   return GdtTssTables;\r
 }\r
 \r
+/**\r
+  This function sets GDT/IDT buffer to be RO and XP.\r
+**/\r
+VOID\r
+PatchGdtIdtMap (\r
+  VOID\r
+  )\r
+{\r
+  EFI_PHYSICAL_ADDRESS       BaseAddress;\r
+  UINTN                      Size;\r
+\r
+  //\r
+  // GDT\r
+  //\r
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n"));\r
+\r
+  BaseAddress = mGdtBuffer;\r
+  Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB);\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_RO\r
+    );\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_XP\r
+    );\r
+\r
+  //\r
+  // IDT\r
+  //\r
+  DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n"));\r
+\r
+  BaseAddress = gcSmiIdtr.Base;\r
+  Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB);\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_RO\r
+    );\r
+  SmmSetMemoryAttributes (\r
+    BaseAddress,\r
+    Size,\r
+    EFI_MEMORY_XP\r
+    );\r
+}\r
+\r
 /**\r
   Get Protected mode code segment from current GDT table.\r
 \r
@@ -154,4 +202,3 @@ TransferApToSafeState (
   ASSERT (FALSE);\r
 }\r
 \r
-\r