UefiCpuPkg/PiSmmCpuDxeSmm: patch "XdSupported" with PatchInstructionX86()

"mXdSupported" is a global BOOLEAN variable, initialized to TRUE. The
CheckFeatureSupported() function is executed on all processors (not
concurrently though), called from SmmInitHandler(). If XD support is found
to be missing on any CPU, then "mXdSupported" is set to FALSE, and further
processors omit the check. Afterwards, "mXdSupported" is read by several
assembly and C code locations.

The tricky part is *where* "mXdSupported" is allocated (defined):

- Before commit 717fb60443fb ("UefiCpuPkg/PiSmmCpuDxeSmm: Add paging
  protection.", 2016-11-17), it used to be a normal global variable,
  defined (allocated) in "SmmProfile.c".

- With said commit, we moved the definition (allocation) of "mXdSupported"
  into "SmiEntry.nasm". The variable was defined over the last byte of a
  "mov al, 1" instruction, so that setting it to FALSE in
  CheckFeatureSupported() would patch the instruction to "mov al, 0". The
  subsequent conditional jump would change behavior, plus all further read
  references to "mXdSupported" (in C and assembly code) would read back
  the source (imm8) operand of the patched MOV instruction as data.

  This trick required that the MOV instruction be encoded with DB.

In order to get rid of the DB, we have to split both roles: we need a
label for the code patching, and "mXdSupported" has to be defined
(allocated) independently of the code patching. Of course, their values
must always remain in sync.

(1) Reinstate the "mXdSupported" definition and initialization in
    "SmmProfile.c" from before commit 717fb60443fb. Change the assembly
    language definition ("global") to a declaration ("extern").

(2) Define the "gPatchXdSupported" label (type X86_ASSEMBLY_PATCH_LABEL)
    in "SmiEntry.nasm", and add the C-language declaration to
    "SmmProfileInternal.h". Replace the DB with the MOV mnemonic (keeping
    the imm8 source operand with value 1).

(3) In CheckFeatureSupported(), whenever "mXdSupported" is set to FALSE,
    patch the assembly code in sync, with PatchInstructionX86().

Cc: Eric Dong <eric.dong@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=866
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Liming Gao <liming.gao@intel.com>

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
index 0023cb328d6a6e52943560058a789bf0b39272ae..509e7a0a665fbc1ead1897955fcdbe44af7cbf5e 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm
@@ -47,7 +47,8 @@ global ASM_PFX(gcSmiHandlerSize)
 global ASM_PFX(gPatchSmiCr3)\r
 global ASM_PFX(gPatchSmiStack)\r
 global ASM_PFX(gPatchSmbase)\r
 global ASM_PFX(gPatchSmiCr3)\r
 global ASM_PFX(gPatchSmiStack)\r
 global ASM_PFX(gPatchSmbase)\r

-global ASM_PFX(mXdSupported)\r
+extern ASM_PFX(mXdSupported)\r
+global ASM_PFX(gPatchXdSupported)\r

 extern ASM_PFX(gSmiHandlerIdtr)\r
 \r
     SECTION .text\r
 extern ASM_PFX(gSmiHandlerIdtr)\r
 \r
     SECTION .text\r


@@ -133,8 +134,8 @@ ASM_PFX(gPatchSmiCr3):
 .6:\r
 \r
 ; enable NXE if supported\r
 .6:\r
 \r
 ; enable NXE if supported\r

-    DB      0b0h                        ; mov al, imm8\r
-ASM_PFX(mXdSupported):     DB      1\r
+    mov     al, strict byte 1           ; source operand may be patched\r
+ASM_PFX(gPatchXdSupported):\r

     cmp     al, 0\r
     jz      @SkipXd\r
 ;\r
     cmp     al, 0\r
     jz      @SkipXd\r
 ;\r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
index c90167f160608fa79bafd663cfb90909181f8052..b4fe0bc23b6cb6f9d0b0a53050d5255700366456 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c
@@ -31,6 +31,11 @@ UINTN                     mSmmProfileSize;
 //\r
 UINTN                     mMsrDsAreaSize   = SMM_PROFILE_DTS_SIZE;\r
 \r
 //\r
 UINTN                     mMsrDsAreaSize   = SMM_PROFILE_DTS_SIZE;\r
 \r

+//\r
+// The flag indicates if execute-disable is supported by processor.\r
+//\r
+BOOLEAN                   mXdSupported     = TRUE;\r
+\r

 //\r
 // The flag indicates if execute-disable is enabled on processor.\r
 //\r
 //\r
 // The flag indicates if execute-disable is enabled on processor.\r
 //\r


@@ -1010,6 +1015,7 @@ CheckFeatureSupported (
       // Extended CPUID functions are not supported on this processor.\r
       //\r
       mXdSupported = FALSE;\r
       // Extended CPUID functions are not supported on this processor.\r
       //\r
       mXdSupported = FALSE;\r

+      PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);\r

     }\r
 \r
     AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx);\r
     }\r
 \r
     AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &RegEdx);\r


@@ -1018,6 +1024,7 @@ CheckFeatureSupported (
       // Execute Disable Bit feature is not supported on this processor.\r
       //\r
       mXdSupported = FALSE;\r
       // Execute Disable Bit feature is not supported on this processor.\r
       //\r
       mXdSupported = FALSE;\r

+      PatchInstructionX86 (gPatchXdSupported, mXdSupported, 1);\r

     }\r
   }\r
 \r
     }\r
   }\r
 \r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
index a21689145bb4eac5e8c33fb12861a384a3cf0602..1613e9cd5cb97f59668d4fdccae29f3a70e98891 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfileInternal.h
@@ -100,6 +100,7 @@ typedef struct {
 extern SMM_S3_RESUME_STATE       *mSmmS3ResumeState;\r
 extern UINTN                     gSmiExceptionHandlers[];\r
 extern BOOLEAN                   mXdSupported;\r
 extern SMM_S3_RESUME_STATE       *mSmmS3ResumeState;\r
 extern UINTN                     gSmiExceptionHandlers[];\r
 extern BOOLEAN                   mXdSupported;\r

+X86_ASSEMBLY_PATCH_LABEL         gPatchXdSupported;\r

 extern UINTN                     *mPFEntryCount;\r
 extern UINT64                    (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];\r
 extern UINT64                    *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];\r
 extern UINTN                     *mPFEntryCount;\r
 extern UINT64                    (*mLastPFEntryValue)[MAX_PF_ENTRY_COUNT];\r
 extern UINT64                    *(*mLastPFEntryPointer)[MAX_PF_ENTRY_COUNT];\r

diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
index 9971ae6f064a8023e4fd0047492fbfd89fa8a3b4..5d731e2280957a418f53b5707a60414ca9b7ab73 100644 (file)
--- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
+++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
@@ -54,7 +54,8 @@ extern ASM_PFX(CpuSmmDebugEntry)
 extern ASM_PFX(CpuSmmDebugExit)\r
 \r
 global ASM_PFX(gPatchSmbase)\r
 extern ASM_PFX(CpuSmmDebugExit)\r
 \r
 global ASM_PFX(gPatchSmbase)\r

-global ASM_PFX(mXdSupported)\r
+extern ASM_PFX(mXdSupported)\r
+global ASM_PFX(gPatchXdSupported)\r

 global ASM_PFX(gPatchSmiStack)\r
 global ASM_PFX(gPatchSmiCr3)\r
 global ASM_PFX(gcSmiHandlerTemplate)\r
 global ASM_PFX(gPatchSmiStack)\r
 global ASM_PFX(gPatchSmiCr3)\r
 global ASM_PFX(gcSmiHandlerTemplate)\r


@@ -118,8 +119,8 @@ ASM_PFX(gPatchSmiCr3):
     ltr     ax\r
 \r
 ; enable NXE if supported\r
     ltr     ax\r
 \r
 ; enable NXE if supported\r

-    DB      0xb0                        ; mov al, imm8\r
-ASM_PFX(mXdSupported):     DB      1\r
+    mov     al, strict byte 1           ; source operand may be patched\r
+ASM_PFX(gPatchXdSupported):\r

     cmp     al, 0\r
     jz      @SkipXd\r
 ;\r
     cmp     al, 0\r
     jz      @SkipXd\r
 ;\r