* [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. @ 2016-11-23 14:54 Jiewen Yao 2016-11-23 22:28 ` Laszlo Ersek 0 siblings, 1 reply; 3+ messages in thread From: Jiewen Yao @ 2016-11-23 14:54 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Jeff Fan, Michael D Kinney 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. This issue does not exist on X64, or IA32 without StackGuard. In this enhanced solution, we create a special GDT whose CS/DS is at read-only page, and whose TSS segment is at read-write page. We also put TSS into a read-write page. The detail is at UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c. Then the normal code can being protected, because it does not touch TSS. When page fault happens, the TSS region can be written. This fix is IA32 StackGuard specific. 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> --- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 4 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 4 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 4 +- UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 207 ++++++++++++++++---- UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 7 +- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 78 +++++++- UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 ----- UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 67 ++++++- 11 files changed, 311 insertions(+), 114 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S index 0c07558..e27080d 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S @@ -41,7 +41,7 @@ ASM_GLOBAL ASM_PFX(gSmiHandlerIdtr) .equ PROTECT_MODE_CS, 0x08 .equ PROTECT_MODE_DS, 0x20 -.equ TSS_SEGMENT, 0x40 +.equ TSS_SEGMENT, 0x1000 .text diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm index eda1708..e1ab470 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm @@ -36,7 +36,7 @@ DSC_OTHERSEG EQU 20 PROTECT_MODE_CS EQU 08h PROTECT_MODE_DS EQU 20h -TSS_SEGMENT EQU 40h +TSS_SEGMENT EQU 1000h SmiRendezvous PROTO C CpuSmmDebugEntry PROTO C diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm index d50a317..ccfb0d4 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm @@ -32,7 +32,7 @@ %define PROTECT_MODE_CS 0x8 %define PROTECT_MODE_DS 0x20 -%define TSS_SEGMENT 0x40 +%define TSS_SEGMENT 0x1000 extern ASM_PFX(SmiRendezvous) extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S index cf5ef82..30dd25e 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S @@ -99,8 +99,8 @@ ExceptionTssSeg: .equ CODE_SEL, CodeSeg32 - NullSeg .equ DATA_SEL, DataSeg32 - NullSeg -.equ TSS_SEL, TssSeg - NullSeg -.equ EXCEPTION_TSS_SEL, ExceptionTssSeg - NullSeg +.equ TSS_SEL, 0x1000 +.equ EXCEPTION_TSS_SEL, 0x1008 # IA32 TSS fields .equ TSS_ESP0, 4 diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm index 7b162f8..740e4a5 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm @@ -102,8 +102,8 @@ ExceptionTssSeg LABEL QWORD CODE_SEL = offset CodeSeg32 - offset NullSeg DATA_SEL = offset DataSeg32 - offset NullSeg -TSS_SEL = offset TssSeg - offset NullSeg -EXCEPTION_TSS_SEL = offset ExceptionTssSeg - offset NullSeg +TSS_SEL EQU 1000h +EXCEPTION_TSS_SEL EQU 1008h IA32_TSS STRUC DW ? diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm index 4d58999..a6dcf47 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm @@ -98,8 +98,8 @@ ExceptionTssSeg: CODE_SEL equ CodeSeg32 - NullSeg DATA_SEL equ DataSeg32 - NullSeg -TSS_SEL equ TssSeg - NullSeg -EXCEPTION_TSS_SEL equ ExceptionTssSeg - NullSeg +%define TSS_SEL 0x1000 +%define EXCEPTION_TSS_SEL 0x1008 struc IA32_TSS resw 1 diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c index f4db6c8..8e25b49 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c @@ -43,17 +43,15 @@ InitializeIDTSmmStackGuard ( /** Initialize Gdt for all processors. - + @param[in] Cr3 CR3 value. - @param[out] GdtStepSize The step size for GDT table. - @return GdtBase for processor 0. - GdtBase for processor X is: GdtBase + (GdtStepSize * X) + @return A GdtArray to hold all base of Gdt Table for all processors. + GdtBase for processor X is: GdtArray[X] **/ -VOID * +UINTN * InitGdt ( - IN UINTN Cr3, - OUT UINTN *GdtStepSize + IN UINTN Cr3 ) { UINTN Index; @@ -62,6 +60,16 @@ InitGdt ( UINTN GdtTssTableSize; UINT8 *GdtTssTables; UINTN GdtTableStepSize; + UINTN *GdtArray; + UINTN GdtCountPerPage; + UINTN BundleCount; + UINTN BundleIndex; + UINT8 *BundleBase; + UINTN GdtIndex; + UINTN CountMax; + + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); + ASSERT (GdtArray != NULL); if (FeaturePcdGet (PcdCpuSmmStackGuard)) { // @@ -71,41 +79,92 @@ InitGdt ( // // - // Enlarge GDT to contain 2 TSS descriptors + // We must separate normal GDT entry from GDT_TSS entry. + // The formal could be read only, and the later must be readwrite. + // The final layout is below: + // + // |<------ Read Only Bundle ------>|<--------- Read Write Bundle -------------->| + // | | | + // +--------------------------------+============================================+--------------+==================+ + // | GDT(0) | GDT(1) | ... | GDT(n) | GDT_TSS(0) | GDT_TSS(1) | ... | GDT_TSS(n) | GDT(n+1) |...| GDT_TSS(n+1) |...| + // +--------------------------------+============================================+--------------+==================+ + // | | | + // |<------------ Gdt.Limit ------------>|<-TSS->| // - gcSmiGdtr.Limit += (UINT16)(2 * sizeof (IA32_SEGMENT_DESCRIPTOR)); - GdtTssTableSize = (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned - mGdtBufferSize = GdtTssTableSize * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; - GdtTssTables = (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES (mGdtBufferSize)); + GdtTableStepSize = (2 * sizeof (IA32_SEGMENT_DESCRIPTOR) + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned + if (GdtTableStepSize < (UINTN)(gcSmiGdtr.Limit + 1)) { + GdtTableStepSize = gcSmiGdtr.Limit + 1; + } + GdtCountPerPage = EFI_PAGE_SIZE / GdtTableStepSize; + BundleCount = (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus + GdtCountPerPage - 1) / GdtCountPerPage; + mGdtBufferSize = EFI_PAGES_TO_SIZE(BundleCount * 2); + + GdtTssTables = (UINT8*)AllocateCodePages (BundleCount * 2); ASSERT (GdtTssTables != NULL); + ZeroMem ((VOID *)GdtTssTables, EFI_PAGES_TO_SIZE(BundleCount * 2)); mGdtBuffer = (UINTN)GdtTssTables; - GdtTableStepSize = GdtTssTableSize; - for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { - CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1 + TSS_SIZE * 2); - // - // Fixup TSS descriptors - // - TssBase = (UINTN)(GdtTssTables + GdtTableStepSize * Index + gcSmiGdtr.Limit + 1); - GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); - - TssBase += TSS_SIZE; - GdtDescriptor++; - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); - // - // Fixup TSS segments - // - // ESP as known good stack - // - *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; - *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; + Index = 0; + for (BundleIndex = 0; BundleIndex < BundleCount; BundleIndex++) { + CountMax = GdtCountPerPage; + if (BundleIndex == BundleCount - 1) { + CountMax = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - GdtCountPerPage * BundleIndex; + } + BundleBase = GdtTssTables + EFI_PAGE_SIZE * 2 * BundleIndex; + for (GdtIndex = 0; GdtIndex < CountMax; GdtIndex++, Index++) { + GdtArray[Index] = (UINTN)BundleBase + GdtTableStepSize * GdtIndex; + // + // Copy Basic GDT + // + CopyMem ( + (VOID *)GdtArray[Index], + (VOID*)(UINTN)gcSmiGdtr.Base, + gcSmiGdtr.Limit + 1 + ); + // + // Copy GDT TSS segment + // + CopyMem ( + (VOID *)(GdtArray[Index] + EFI_PAGE_SIZE), + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1), + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2 + ); + // + // Copy TSS + // + TssBase = (UINTN)(GdtArray[Index] + EFI_PAGE_SIZE + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2); + CopyMem ( + (VOID *)(TssBase), + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1 + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2), + TSS_SIZE * 2 + ); + // + // Fixup TSS descriptors + // + GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); + + TssBase += TSS_SIZE; + GdtDescriptor++; + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); + // + // Fixup TSS segments + // + // ESP as known good stack + // + *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; + *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; + } } + // + // Enlarge GDT to contain 1 page plus 2 TSS descriptors + // + gcSmiGdtr.Limit = (UINT16)(EFI_PAGE_SIZE + 2 * sizeof (IA32_SEGMENT_DESCRIPTOR) - 1); } else { // // Just use original table, AllocatePage and copy them here to make sure GDTs are covered in page memory. @@ -119,11 +178,83 @@ InitGdt ( for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1); + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; + } + } + + return GdtArray; +} + +/** + This function sets GDT/IDT buffer to be RO and XP. +**/ +VOID +PatchGdtIdtMap ( + VOID + ) +{ + EFI_PHYSICAL_ADDRESS BaseAddress; + UINTN Size; + UINTN Index; + UINTN BundleCount; + + // + // GDT + // + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n")); + + if (FeaturePcdGet (PcdCpuSmmStackGuard)) { + BaseAddress = mGdtBuffer; + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); + SmmSetMemoryAttributes ( + BaseAddress, + Size, + EFI_MEMORY_XP + ); + BundleCount = EFI_SIZE_TO_PAGES(mGdtBufferSize)/2; + for (Index = 0; Index < BundleCount; Index++) { + SmmSetMemoryAttributes ( + BaseAddress + EFI_PAGE_SIZE * Index * 2, + EFI_PAGE_SIZE, + EFI_MEMORY_RO + ); + SmmClearMemoryAttributes ( + BaseAddress + EFI_PAGE_SIZE * (Index * 2 + 1), + EFI_PAGE_SIZE, + EFI_MEMORY_RO + ); } + } else { + BaseAddress = mGdtBuffer; + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); + SmmSetMemoryAttributes ( + BaseAddress, + Size, + EFI_MEMORY_RO + ); + SmmSetMemoryAttributes ( + BaseAddress, + Size, + EFI_MEMORY_XP + ); } + // + // IDT + // + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n")); - *GdtStepSize = GdtTableStepSize; - return GdtTssTables; + BaseAddress = gcSmiIdtr.Base; + Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB); + SmmSetMemoryAttributes ( + BaseAddress, + Size, + EFI_MEMORY_RO + ); + SmmSetMemoryAttributes ( + BaseAddress, + Size, + EFI_MEMORY_XP + ); } /** diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c index 9b8db90..591a18c 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c @@ -1377,8 +1377,7 @@ InitializeMpServiceData ( UINTN Index; MTRR_SETTINGS *Mtrr; PROCESSOR_SMM_DESCRIPTOR *Psd; - UINT8 *GdtTssTables; - UINTN GdtTableStepSize; + UINTN *GdtArray; // // Allocate memory for all locks and semaphores @@ -1407,7 +1406,7 @@ InitializeMpServiceData ( // Cr3 = SmmInitPageTable (); - GdtTssTables = InitGdt (Cr3, &GdtTableStepSize); + GdtArray = InitGdt (Cr3); // // Initialize PROCESSOR_SMM_DESCRIPTOR for each CPU @@ -1415,7 +1414,7 @@ InitializeMpServiceData ( for (Index = 0; Index < mMaxNumberOfCpus; Index++) { Psd = (PROCESSOR_SMM_DESCRIPTOR *)(VOID *)(UINTN)(mCpuHotPlugData.SmBase[Index] + SMM_PSD_OFFSET); CopyMem (Psd, &gcPsd, sizeof (gcPsd)); - Psd->SmmGdtPtr = (UINT64)(UINTN)(GdtTssTables + GdtTableStepSize * Index); + Psd->SmmGdtPtr = (UINT64)(UINTN)GdtArray[Index]; Psd->SmmGdtSize = gcSmiGdtr.Limit + 1; // diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h index abe5cc6..9e00022 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -512,15 +512,21 @@ InitializeIDTSmmStackGuard ( Initialize Gdt for all processors. @param[in] Cr3 CR3 value. - @param[out] GdtStepSize The step size for GDT table. - @return GdtBase for processor 0. - GdtBase for processor X is: GdtBase + (GdtStepSize * X) + @return A GdtArray to hold all base of Gdt Table for all processors. + GdtBase for processor X is: GdtArray[X] **/ -VOID * +UINTN * InitGdt ( - IN UINTN Cr3, - OUT UINTN *GdtStepSize + IN UINTN Cr3 + ); + +/** + This function sets GDT/IDT buffer to be RO and XP. +**/ +VOID +PatchGdtIdtMap ( + VOID ); /** @@ -596,6 +602,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 EFI_PHYSICAL_ADDRESS BaseAddress, + IN UINT64 Length, + IN UINT64 Attributes + ); + +/** + This function clears 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 clear for the memory region. + + @retval EFI_SUCCESS The attributes were cleared 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 +SmmClearMemoryAttributes ( + IN EFI_PHYSICAL_ADDRESS BaseAddress, + IN UINT64 Length, + IN UINT64 Attributes + ); + +/** Initialize MP synchronization data. **/ diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index c85e025..fc440e4 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c @@ -750,54 +750,6 @@ PatchSmmSaveStateMap ( } /** - 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); - 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 - ); -} - -/** This function sets memory attribute according to MemoryAttributesTable. **/ VOID diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c index 9fc00c1..baeec4a 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c @@ -42,15 +42,13 @@ InitializeIDTSmmStackGuard ( Initialize Gdt for all processors. @param[in] Cr3 CR3 value. - @param[out] GdtStepSize The step size for GDT table. - @return GdtBase for processor 0. - GdtBase for processor X is: GdtBase + (GdtStepSize * X) + @return A GdtArray to hold all base of Gdt Table for all processors. + GdtBase for processor X is: GdtArray[X] **/ -VOID * +UINTN * InitGdt ( - IN UINTN Cr3, - OUT UINTN *GdtStepSize + IN UINTN Cr3 ) { UINTN Index; @@ -59,6 +57,10 @@ InitGdt ( UINTN GdtTssTableSize; UINT8 *GdtTssTables; UINTN GdtTableStepSize; + UINTN *GdtArray; + + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); + ASSERT (GdtArray != NULL); // // For X64 SMM, we allocate separate GDT/TSS for each CPUs to avoid TSS load contention @@ -89,10 +91,58 @@ InitGdt ( // *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize); } + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; } - *GdtStepSize = GdtTableStepSize; - return GdtTssTables; + return GdtArray; +} + +/** + 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); + 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 + ); } /** @@ -154,4 +204,3 @@ TransferApToSafeState ( ASSERT (FALSE); } - -- 2.7.4.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. 2016-11-23 14:54 [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault Jiewen Yao @ 2016-11-23 22:28 ` Laszlo Ersek [not found] ` <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com> 0 siblings, 1 reply; 3+ messages in thread From: Laszlo Ersek @ 2016-11-23 22:28 UTC (permalink / raw) To: Jiewen Yao, edk2-devel; +Cc: Michael D Kinney, Jeff Fan On 11/23/16 15:54, 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. > > This issue does not exist on X64, or IA32 without StackGuard. > > In this enhanced solution, we create a special GDT whose CS/DS is > at read-only page, and whose TSS segment is at read-write page. > We also put TSS into a read-write page. > > The detail is at UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c. > > Then the normal code can being protected, because it does not touch TSS. > When page fault happens, the TSS region can be written. > > This fix is IA32 StackGuard specific. > > 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> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 207 ++++++++++++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 7 +- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 78 +++++++- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 ----- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 67 ++++++- > 11 files changed, 311 insertions(+), 114 deletions(-) I'm not volunteering for a review (I have no clue what a task switch is). I've done regression testing with Ia32 and Ia32X64 OVMF, including repeated suspend/resume. Regression-tested-by: Laszlo Ersek <lersek@redhat.com> In addition, let me point out that my recent testing of the patch that improves the SMM exception messages: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message [edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. covered the SMM stack overflow case, but -- as I stated explicitly -- only in the Ia32X64 build. See bullet (2) in <https://lists.01.org/pipermail/edk2-devel/2016-November/005078.html>. The reason is that the SMM stack overflow has occurred to us only when running the EnrollDefaultKeys.efi application, and for that app we only have a "production use" under Ia32X64, not Ia32. Anyway, I have now applied your [edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. together with this patch, and attempted to trigger SMM stack overflow on both Ia32 and Ia32X64. - I confirm that the Ia32X64 case continues to work, like before: <https://lists.01.org/pipermail/edk2-devel/2016-November/005133.html>. For that, Regression-tested-by: Laszlo Ersek <lersek@redhat.com> - Regarding the Ia32 case, which I haven't tested before, I couldn't trigger an SMM stack overflow with EnrollDefaultKeys.efi, even after reverting the patch that increased the SMM stack size in OVMF (0d0c245dfb14). So, I think I cannot give my Tested-by (I couldn't test the actual new functionality), just my Regression-tested-by (see above). Thanks Laszlo > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > index 0c07558..e27080d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > @@ -41,7 +41,7 @@ ASM_GLOBAL ASM_PFX(gSmiHandlerIdtr) > > .equ PROTECT_MODE_CS, 0x08 > .equ PROTECT_MODE_DS, 0x20 > -.equ TSS_SEGMENT, 0x40 > +.equ TSS_SEGMENT, 0x1000 > > .text > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > index eda1708..e1ab470 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > @@ -36,7 +36,7 @@ DSC_OTHERSEG EQU 20 > > PROTECT_MODE_CS EQU 08h > PROTECT_MODE_DS EQU 20h > -TSS_SEGMENT EQU 40h > +TSS_SEGMENT EQU 1000h > > SmiRendezvous PROTO C > CpuSmmDebugEntry PROTO C > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > index d50a317..ccfb0d4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > @@ -32,7 +32,7 @@ > > %define PROTECT_MODE_CS 0x8 > %define PROTECT_MODE_DS 0x20 > -%define TSS_SEGMENT 0x40 > +%define TSS_SEGMENT 0x1000 > > extern ASM_PFX(SmiRendezvous) > extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > index cf5ef82..30dd25e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > @@ -99,8 +99,8 @@ ExceptionTssSeg: > > .equ CODE_SEL, CodeSeg32 - NullSeg > .equ DATA_SEL, DataSeg32 - NullSeg > -.equ TSS_SEL, TssSeg - NullSeg > -.equ EXCEPTION_TSS_SEL, ExceptionTssSeg - NullSeg > +.equ TSS_SEL, 0x1000 > +.equ EXCEPTION_TSS_SEL, 0x1008 > > # IA32 TSS fields > .equ TSS_ESP0, 4 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > index 7b162f8..740e4a5 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > @@ -102,8 +102,8 @@ ExceptionTssSeg LABEL QWORD > > CODE_SEL = offset CodeSeg32 - offset NullSeg > DATA_SEL = offset DataSeg32 - offset NullSeg > -TSS_SEL = offset TssSeg - offset NullSeg > -EXCEPTION_TSS_SEL = offset ExceptionTssSeg - offset NullSeg > +TSS_SEL EQU 1000h > +EXCEPTION_TSS_SEL EQU 1008h > > IA32_TSS STRUC > DW ? > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > index 4d58999..a6dcf47 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > @@ -98,8 +98,8 @@ ExceptionTssSeg: > > CODE_SEL equ CodeSeg32 - NullSeg > DATA_SEL equ DataSeg32 - NullSeg > -TSS_SEL equ TssSeg - NullSeg > -EXCEPTION_TSS_SEL equ ExceptionTssSeg - NullSeg > +%define TSS_SEL 0x1000 > +%define EXCEPTION_TSS_SEL 0x1008 > > struc IA32_TSS > resw 1 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index f4db6c8..8e25b49 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -43,17 +43,15 @@ InitializeIDTSmmStackGuard ( > > /** > Initialize Gdt for all processors. > - > + > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > ) > { > UINTN Index; > @@ -62,6 +60,16 @@ InitGdt ( > UINTN GdtTssTableSize; > UINT8 *GdtTssTables; > UINTN GdtTableStepSize; > + UINTN *GdtArray; > + UINTN GdtCountPerPage; > + UINTN BundleCount; > + UINTN BundleIndex; > + UINT8 *BundleBase; > + UINTN GdtIndex; > + UINTN CountMax; > + > + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > + ASSERT (GdtArray != NULL); > > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > // > @@ -71,41 +79,92 @@ InitGdt ( > // > > // > - // Enlarge GDT to contain 2 TSS descriptors > + // We must separate normal GDT entry from GDT_TSS entry. > + // The formal could be read only, and the later must be readwrite. > + // The final layout is below: > + // > + // |<------ Read Only Bundle ------>|<--------- Read Write Bundle -------------->| > + // | | | > + // +--------------------------------+============================================+--------------+==================+ > + // | GDT(0) | GDT(1) | ... | GDT(n) | GDT_TSS(0) | GDT_TSS(1) | ... | GDT_TSS(n) | GDT(n+1) |...| GDT_TSS(n+1) |...| > + // +--------------------------------+============================================+--------------+==================+ > + // | | | > + // |<------------ Gdt.Limit ------------>|<-TSS->| > // > - gcSmiGdtr.Limit += (UINT16)(2 * sizeof (IA32_SEGMENT_DESCRIPTOR)); > > - GdtTssTableSize = (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned > - mGdtBufferSize = GdtTssTableSize * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > - GdtTssTables = (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES (mGdtBufferSize)); > + GdtTableStepSize = (2 * sizeof (IA32_SEGMENT_DESCRIPTOR) + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned > + if (GdtTableStepSize < (UINTN)(gcSmiGdtr.Limit + 1)) { > + GdtTableStepSize = gcSmiGdtr.Limit + 1; > + } > + GdtCountPerPage = EFI_PAGE_SIZE / GdtTableStepSize; > + BundleCount = (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus + GdtCountPerPage - 1) / GdtCountPerPage; > + mGdtBufferSize = EFI_PAGES_TO_SIZE(BundleCount * 2); > + > + GdtTssTables = (UINT8*)AllocateCodePages (BundleCount * 2); > ASSERT (GdtTssTables != NULL); > + ZeroMem ((VOID *)GdtTssTables, EFI_PAGES_TO_SIZE(BundleCount * 2)); > mGdtBuffer = (UINTN)GdtTssTables; > - GdtTableStepSize = GdtTssTableSize; > > - for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { > - CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1 + TSS_SIZE * 2); > - // > - // Fixup TSS descriptors > - // > - TssBase = (UINTN)(GdtTssTables + GdtTableStepSize * Index + gcSmiGdtr.Limit + 1); > - GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > - > - TssBase += TSS_SIZE; > - GdtDescriptor++; > - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > - // > - // Fixup TSS segments > - // > - // ESP as known good stack > - // > - *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; > - *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; > + Index = 0; > + for (BundleIndex = 0; BundleIndex < BundleCount; BundleIndex++) { > + CountMax = GdtCountPerPage; > + if (BundleIndex == BundleCount - 1) { > + CountMax = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - GdtCountPerPage * BundleIndex; > + } > + BundleBase = GdtTssTables + EFI_PAGE_SIZE * 2 * BundleIndex; > + for (GdtIndex = 0; GdtIndex < CountMax; GdtIndex++, Index++) { > + GdtArray[Index] = (UINTN)BundleBase + GdtTableStepSize * GdtIndex; > + // > + // Copy Basic GDT > + // > + CopyMem ( > + (VOID *)GdtArray[Index], > + (VOID*)(UINTN)gcSmiGdtr.Base, > + gcSmiGdtr.Limit + 1 > + ); > + // > + // Copy GDT TSS segment > + // > + CopyMem ( > + (VOID *)(GdtArray[Index] + EFI_PAGE_SIZE), > + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1), > + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2 > + ); > + // > + // Copy TSS > + // > + TssBase = (UINTN)(GdtArray[Index] + EFI_PAGE_SIZE + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2); > + CopyMem ( > + (VOID *)(TssBase), > + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1 + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2), > + TSS_SIZE * 2 > + ); > + // > + // Fixup TSS descriptors > + // > + GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > + > + TssBase += TSS_SIZE; > + GdtDescriptor++; > + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > + // > + // Fixup TSS segments > + // > + // ESP as known good stack > + // > + *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; > + *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; > + } > } > + // > + // Enlarge GDT to contain 1 page plus 2 TSS descriptors > + // > + gcSmiGdtr.Limit = (UINT16)(EFI_PAGE_SIZE + 2 * sizeof (IA32_SEGMENT_DESCRIPTOR) - 1); > } else { > // > // Just use original table, AllocatePage and copy them here to make sure GDTs are covered in page memory. > @@ -119,11 +178,83 @@ InitGdt ( > > for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { > CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1); > + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; > + } > + } > + > + return GdtArray; > +} > + > +/** > + This function sets GDT/IDT buffer to be RO and XP. > +**/ > +VOID > +PatchGdtIdtMap ( > + VOID > + ) > +{ > + EFI_PHYSICAL_ADDRESS BaseAddress; > + UINTN Size; > + UINTN Index; > + UINTN BundleCount; > + > + // > + // GDT > + // > + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n")); > + > + if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > + BaseAddress = mGdtBuffer; > + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > + BundleCount = EFI_SIZE_TO_PAGES(mGdtBufferSize)/2; > + for (Index = 0; Index < BundleCount; Index++) { > + SmmSetMemoryAttributes ( > + BaseAddress + EFI_PAGE_SIZE * Index * 2, > + EFI_PAGE_SIZE, > + EFI_MEMORY_RO > + ); > + SmmClearMemoryAttributes ( > + BaseAddress + EFI_PAGE_SIZE * (Index * 2 + 1), > + EFI_PAGE_SIZE, > + EFI_MEMORY_RO > + ); > } > + } else { > + BaseAddress = mGdtBuffer; > + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_RO > + ); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > } > + // > + // IDT > + // > + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n")); > > - *GdtStepSize = GdtTableStepSize; > - return GdtTssTables; > + BaseAddress = gcSmiIdtr.Base; > + Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_RO > + ); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 9b8db90..591a18c 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1377,8 +1377,7 @@ InitializeMpServiceData ( > UINTN Index; > MTRR_SETTINGS *Mtrr; > PROCESSOR_SMM_DESCRIPTOR *Psd; > - UINT8 *GdtTssTables; > - UINTN GdtTableStepSize; > + UINTN *GdtArray; > > // > // Allocate memory for all locks and semaphores > @@ -1407,7 +1406,7 @@ InitializeMpServiceData ( > // > Cr3 = SmmInitPageTable (); > > - GdtTssTables = InitGdt (Cr3, &GdtTableStepSize); > + GdtArray = InitGdt (Cr3); > > // > // Initialize PROCESSOR_SMM_DESCRIPTOR for each CPU > @@ -1415,7 +1414,7 @@ InitializeMpServiceData ( > for (Index = 0; Index < mMaxNumberOfCpus; Index++) { > Psd = (PROCESSOR_SMM_DESCRIPTOR *)(VOID *)(UINTN)(mCpuHotPlugData.SmBase[Index] + SMM_PSD_OFFSET); > CopyMem (Psd, &gcPsd, sizeof (gcPsd)); > - Psd->SmmGdtPtr = (UINT64)(UINTN)(GdtTssTables + GdtTableStepSize * Index); > + Psd->SmmGdtPtr = (UINT64)(UINTN)GdtArray[Index]; > Psd->SmmGdtSize = gcSmiGdtr.Limit + 1; > > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index abe5cc6..9e00022 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -512,15 +512,21 @@ InitializeIDTSmmStackGuard ( > Initialize Gdt for all processors. > > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > + ); > + > +/** > + This function sets GDT/IDT buffer to be RO and XP. > +**/ > +VOID > +PatchGdtIdtMap ( > + VOID > ); > > /** > @@ -596,6 +602,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 EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length, > + IN UINT64 Attributes > + ); > + > +/** > + This function clears 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 clear for the memory region. > + > + @retval EFI_SUCCESS The attributes were cleared 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 > +SmmClearMemoryAttributes ( > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length, > + IN UINT64 Attributes > + ); > + > +/** > Initialize MP synchronization data. > > **/ > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index c85e025..fc440e4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -750,54 +750,6 @@ PatchSmmSaveStateMap ( > } > > /** > - 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); > - 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 > - ); > -} > - > -/** > This function sets memory attribute according to MemoryAttributesTable. > **/ > VOID > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index 9fc00c1..baeec4a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -42,15 +42,13 @@ InitializeIDTSmmStackGuard ( > Initialize Gdt for all processors. > > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > ) > { > UINTN Index; > @@ -59,6 +57,10 @@ InitGdt ( > UINTN GdtTssTableSize; > UINT8 *GdtTssTables; > UINTN GdtTableStepSize; > + UINTN *GdtArray; > + > + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > + ASSERT (GdtArray != NULL); > > // > // For X64 SMM, we allocate separate GDT/TSS for each CPUs to avoid TSS load contention > @@ -89,10 +91,58 @@ InitGdt ( > // > *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize); > } > + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; > } > > - *GdtStepSize = GdtTableStepSize; > - return GdtTssTables; > + return GdtArray; > +} > + > +/** > + 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); > + 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 > + ); > } > > /** > @@ -154,4 +204,3 @@ TransferApToSafeState ( > ASSERT (FALSE); > } > > - > ^ permalink raw reply [flat|nested] 3+ messages in thread
[parent not found: <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com>]
* Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. [not found] ` <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com> @ 2016-11-24 0:45 ` Yao, Jiewen 0 siblings, 0 replies; 3+ messages in thread From: Yao, Jiewen @ 2016-11-24 0:45 UTC (permalink / raw) To: edk2-devel@ml01.01.org, Laszlo Ersek (lersek@redhat.com) Cc: Kinney, Michael D, Fan, Jeff Thank you, Laszlo. I will check in debug message enhancement at first, with your suggestion to remove the last superfluous argument and use small 'x' consistently. :) Your regression test is very nice. For this patch, I will invite more people to review and validation to see if there is other concern. Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek Sent: Thursday, November 24, 2016 6:29 AM To: Yao, Jiewen; edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org> Cc: Kinney, Michael D; Fan, Jeff Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. On 11/23/16 15:54, 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. > > This issue does not exist on X64, or IA32 without StackGuard. > > In this enhanced solution, we create a special GDT whose CS/DS is > at read-only page, and whose TSS segment is at read-write page. > We also put TSS into a read-write page. > > The detail is at UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c. > > Then the normal code can being protected, because it does not touch TSS. > When page fault happens, the TSS region can be written. > > This fix is IA32 StackGuard specific. > > Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> > Cc: Jeff Fan <jeff.fan@intel.com<mailto:jeff.fan@intel.com>> > Cc: Michael D Kinney <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm | 2 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm | 4 +- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 207 ++++++++++++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 7 +- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 78 +++++++- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 ----- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 67 ++++++- > 11 files changed, 311 insertions(+), 114 deletions(-) I'm not volunteering for a review (I have no clue what a task switch is). I've done regression testing with Ia32 and Ia32X64 OVMF, including repeated suspend/resume. Regression-tested-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> In addition, let me point out that my recent testing of the patch that improves the SMM exception messages: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Correct exception message [edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. covered the SMM stack overflow case, but -- as I stated explicitly -- only in the Ia32X64 build. See bullet (2) in <https://lists.01.org/pipermail/edk2-devel/2016-November/005078.html>. The reason is that the SMM stack overflow has occurred to us only when running the EnrollDefaultKeys.efi application, and for that app we only have a "production use" under Ia32X64, not Ia32. Anyway, I have now applied your [edk2] [PATCH V3] UefiCpuPkg/PiSmmCpu: Correct exception message. together with this patch, and attempted to trigger SMM stack overflow on both Ia32 and Ia32X64. - I confirm that the Ia32X64 case continues to work, like before: <https://lists.01.org/pipermail/edk2-devel/2016-November/005133.html>. For that, Regression-tested-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> - Regarding the Ia32 case, which I haven't tested before, I couldn't trigger an SMM stack overflow with EnrollDefaultKeys.efi, even after reverting the patch that increased the SMM stack size in OVMF (0d0c245dfb14). So, I think I cannot give my Tested-by (I couldn't test the actual new functionality), just my Regression-tested-by (see above). Thanks Laszlo > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > index 0c07558..e27080d 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.S > @@ -41,7 +41,7 @@ ASM_GLOBAL ASM_PFX(gSmiHandlerIdtr) > > .equ PROTECT_MODE_CS, 0x08 > .equ PROTECT_MODE_DS, 0x20 > -.equ TSS_SEGMENT, 0x40 > +.equ TSS_SEGMENT, 0x1000 > > .text > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > index eda1708..e1ab470 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.asm > @@ -36,7 +36,7 @@ DSC_OTHERSEG EQU 20 > > PROTECT_MODE_CS EQU 08h > PROTECT_MODE_DS EQU 20h > -TSS_SEGMENT EQU 40h > +TSS_SEGMENT EQU 1000h > > SmiRendezvous PROTO C > CpuSmmDebugEntry PROTO C > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > index d50a317..ccfb0d4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiEntry.nasm > @@ -32,7 +32,7 @@ > > %define PROTECT_MODE_CS 0x8 > %define PROTECT_MODE_DS 0x20 > -%define TSS_SEGMENT 0x40 > +%define TSS_SEGMENT 0x1000 > > extern ASM_PFX(SmiRendezvous) > extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard)) > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > index cf5ef82..30dd25e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.S > @@ -99,8 +99,8 @@ ExceptionTssSeg: > > .equ CODE_SEL, CodeSeg32 - NullSeg > .equ DATA_SEL, DataSeg32 - NullSeg > -.equ TSS_SEL, TssSeg - NullSeg > -.equ EXCEPTION_TSS_SEL, ExceptionTssSeg - NullSeg > +.equ TSS_SEL, 0x1000 > +.equ EXCEPTION_TSS_SEL, 0x1008 > > # IA32 TSS fields > .equ TSS_ESP0, 4 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > index 7b162f8..740e4a5 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.asm > @@ -102,8 +102,8 @@ ExceptionTssSeg LABEL QWORD > > CODE_SEL = offset CodeSeg32 - offset NullSeg > DATA_SEL = offset DataSeg32 - offset NullSeg > -TSS_SEL = offset TssSeg - offset NullSeg > -EXCEPTION_TSS_SEL = offset ExceptionTssSeg - offset NullSeg > +TSS_SEL EQU 1000h > +EXCEPTION_TSS_SEL EQU 1008h > > IA32_TSS STRUC > DW ? > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > index 4d58999..a6dcf47 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmiException.nasm > @@ -98,8 +98,8 @@ ExceptionTssSeg: > > CODE_SEL equ CodeSeg32 - NullSeg > DATA_SEL equ DataSeg32 - NullSeg > -TSS_SEL equ TssSeg - NullSeg > -EXCEPTION_TSS_SEL equ ExceptionTssSeg - NullSeg > +%define TSS_SEL 0x1000 > +%define EXCEPTION_TSS_SEL 0x1008 > > struc IA32_TSS > resw 1 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index f4db6c8..8e25b49 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -43,17 +43,15 @@ InitializeIDTSmmStackGuard ( > > /** > Initialize Gdt for all processors. > - > + > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > ) > { > UINTN Index; > @@ -62,6 +60,16 @@ InitGdt ( > UINTN GdtTssTableSize; > UINT8 *GdtTssTables; > UINTN GdtTableStepSize; > + UINTN *GdtArray; > + UINTN GdtCountPerPage; > + UINTN BundleCount; > + UINTN BundleIndex; > + UINT8 *BundleBase; > + UINTN GdtIndex; > + UINTN CountMax; > + > + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > + ASSERT (GdtArray != NULL); > > if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > // > @@ -71,41 +79,92 @@ InitGdt ( > // > > // > - // Enlarge GDT to contain 2 TSS descriptors > + // We must separate normal GDT entry from GDT_TSS entry. > + // The formal could be read only, and the later must be readwrite. > + // The final layout is below: > + // > + // |<------ Read Only Bundle ------>|<--------- Read Write Bundle -------------->| > + // | | | > + // +--------------------------------+============================================+--------------+==================+ > + // | GDT(0) | GDT(1) | ... | GDT(n) | GDT_TSS(0) | GDT_TSS(1) | ... | GDT_TSS(n) | GDT(n+1) |...| GDT_TSS(n+1) |...| > + // +--------------------------------+============================================+--------------+==================+ > + // | | | > + // |<------------ Gdt.Limit ------------>|<-TSS->| > // > - gcSmiGdtr.Limit += (UINT16)(2 * sizeof (IA32_SEGMENT_DESCRIPTOR)); > > - GdtTssTableSize = (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned > - mGdtBufferSize = GdtTssTableSize * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > - GdtTssTables = (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES (mGdtBufferSize)); > + GdtTableStepSize = (2 * sizeof (IA32_SEGMENT_DESCRIPTOR) + TSS_SIZE * 2 + 7) & ~7; // 8 bytes aligned > + if (GdtTableStepSize < (UINTN)(gcSmiGdtr.Limit + 1)) { > + GdtTableStepSize = gcSmiGdtr.Limit + 1; > + } > + GdtCountPerPage = EFI_PAGE_SIZE / GdtTableStepSize; > + BundleCount = (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus + GdtCountPerPage - 1) / GdtCountPerPage; > + mGdtBufferSize = EFI_PAGES_TO_SIZE(BundleCount * 2); > + > + GdtTssTables = (UINT8*)AllocateCodePages (BundleCount * 2); > ASSERT (GdtTssTables != NULL); > + ZeroMem ((VOID *)GdtTssTables, EFI_PAGES_TO_SIZE(BundleCount * 2)); > mGdtBuffer = (UINTN)GdtTssTables; > - GdtTableStepSize = GdtTssTableSize; > > - for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { > - CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1 + TSS_SIZE * 2); > - // > - // Fixup TSS descriptors > - // > - TssBase = (UINTN)(GdtTssTables + GdtTableStepSize * Index + gcSmiGdtr.Limit + 1); > - GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > - > - TssBase += TSS_SIZE; > - GdtDescriptor++; > - GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > - // > - // Fixup TSS segments > - // > - // ESP as known good stack > - // > - *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; > - *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; > + Index = 0; > + for (BundleIndex = 0; BundleIndex < BundleCount; BundleIndex++) { > + CountMax = GdtCountPerPage; > + if (BundleIndex == BundleCount - 1) { > + CountMax = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - GdtCountPerPage * BundleIndex; > + } > + BundleBase = GdtTssTables + EFI_PAGE_SIZE * 2 * BundleIndex; > + for (GdtIndex = 0; GdtIndex < CountMax; GdtIndex++, Index++) { > + GdtArray[Index] = (UINTN)BundleBase + GdtTableStepSize * GdtIndex; > + // > + // Copy Basic GDT > + // > + CopyMem ( > + (VOID *)GdtArray[Index], > + (VOID*)(UINTN)gcSmiGdtr.Base, > + gcSmiGdtr.Limit + 1 > + ); > + // > + // Copy GDT TSS segment > + // > + CopyMem ( > + (VOID *)(GdtArray[Index] + EFI_PAGE_SIZE), > + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1), > + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2 > + ); > + // > + // Copy TSS > + // > + TssBase = (UINTN)(GdtArray[Index] + EFI_PAGE_SIZE + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2); > + CopyMem ( > + (VOID *)(TssBase), > + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1 + sizeof(IA32_SEGMENT_DESCRIPTOR) * 2), > + TSS_SIZE * 2 > + ); > + // > + // Fixup TSS descriptors > + // > + GdtDescriptor = (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > + > + TssBase += TSS_SIZE; > + GdtDescriptor++; > + GdtDescriptor->Bits.BaseLow = (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid = (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh = (UINT8)(TssBase >> 24); > + // > + // Fixup TSS segments > + // > + // ESP as known good stack > + // > + *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) = mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize; > + *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) = Cr3; > + } > } > + // > + // Enlarge GDT to contain 1 page plus 2 TSS descriptors > + // > + gcSmiGdtr.Limit = (UINT16)(EFI_PAGE_SIZE + 2 * sizeof (IA32_SEGMENT_DESCRIPTOR) - 1); > } else { > // > // Just use original table, AllocatePage and copy them here to make sure GDTs are covered in page memory. > @@ -119,11 +178,83 @@ InitGdt ( > > for (Index = 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; Index++) { > CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gcSmiGdtr.Base, gcSmiGdtr.Limit + 1); > + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; > + } > + } > + > + return GdtArray; > +} > + > +/** > + This function sets GDT/IDT buffer to be RO and XP. > +**/ > +VOID > +PatchGdtIdtMap ( > + VOID > + ) > +{ > + EFI_PHYSICAL_ADDRESS BaseAddress; > + UINTN Size; > + UINTN Index; > + UINTN BundleCount; > + > + // > + // GDT > + // > + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - GDT:\n")); > + > + if (FeaturePcdGet (PcdCpuSmmStackGuard)) { > + BaseAddress = mGdtBuffer; > + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > + BundleCount = EFI_SIZE_TO_PAGES(mGdtBufferSize)/2; > + for (Index = 0; Index < BundleCount; Index++) { > + SmmSetMemoryAttributes ( > + BaseAddress + EFI_PAGE_SIZE * Index * 2, > + EFI_PAGE_SIZE, > + EFI_MEMORY_RO > + ); > + SmmClearMemoryAttributes ( > + BaseAddress + EFI_PAGE_SIZE * (Index * 2 + 1), > + EFI_PAGE_SIZE, > + EFI_MEMORY_RO > + ); > } > + } else { > + BaseAddress = mGdtBuffer; > + Size = ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_RO > + ); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > } > + // > + // IDT > + // > + DEBUG ((DEBUG_INFO, "PatchGdtIdtMap - IDT:\n")); > > - *GdtStepSize = GdtTableStepSize; > - return GdtTssTables; > + BaseAddress = gcSmiIdtr.Base; > + Size = ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_RO > + ); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > } > > /** > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 9b8db90..591a18c 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -1377,8 +1377,7 @@ InitializeMpServiceData ( > UINTN Index; > MTRR_SETTINGS *Mtrr; > PROCESSOR_SMM_DESCRIPTOR *Psd; > - UINT8 *GdtTssTables; > - UINTN GdtTableStepSize; > + UINTN *GdtArray; > > // > // Allocate memory for all locks and semaphores > @@ -1407,7 +1406,7 @@ InitializeMpServiceData ( > // > Cr3 = SmmInitPageTable (); > > - GdtTssTables = InitGdt (Cr3, &GdtTableStepSize); > + GdtArray = InitGdt (Cr3); > > // > // Initialize PROCESSOR_SMM_DESCRIPTOR for each CPU > @@ -1415,7 +1414,7 @@ InitializeMpServiceData ( > for (Index = 0; Index < mMaxNumberOfCpus; Index++) { > Psd = (PROCESSOR_SMM_DESCRIPTOR *)(VOID *)(UINTN)(mCpuHotPlugData.SmBase[Index] + SMM_PSD_OFFSET); > CopyMem (Psd, &gcPsd, sizeof (gcPsd)); > - Psd->SmmGdtPtr = (UINT64)(UINTN)(GdtTssTables + GdtTableStepSize * Index); > + Psd->SmmGdtPtr = (UINT64)(UINTN)GdtArray[Index]; > Psd->SmmGdtSize = gcSmiGdtr.Limit + 1; > > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index abe5cc6..9e00022 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -512,15 +512,21 @@ InitializeIDTSmmStackGuard ( > Initialize Gdt for all processors. > > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > + ); > + > +/** > + This function sets GDT/IDT buffer to be RO and XP. > +**/ > +VOID > +PatchGdtIdtMap ( > + VOID > ); > > /** > @@ -596,6 +602,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 EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length, > + IN UINT64 Attributes > + ); > + > +/** > + This function clears 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 clear for the memory region. > + > + @retval EFI_SUCCESS The attributes were cleared 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 > +SmmClearMemoryAttributes ( > + IN EFI_PHYSICAL_ADDRESS BaseAddress, > + IN UINT64 Length, > + IN UINT64 Attributes > + ); > + > +/** > Initialize MP synchronization data. > > **/ > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index c85e025..fc440e4 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -750,54 +750,6 @@ PatchSmmSaveStateMap ( > } > > /** > - 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); > - 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 > - ); > -} > - > -/** > This function sets memory attribute according to MemoryAttributesTable. > **/ > VOID > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index 9fc00c1..baeec4a 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -42,15 +42,13 @@ InitializeIDTSmmStackGuard ( > Initialize Gdt for all processors. > > @param[in] Cr3 CR3 value. > - @param[out] GdtStepSize The step size for GDT table. > > - @return GdtBase for processor 0. > - GdtBase for processor X is: GdtBase + (GdtStepSize * X) > + @return A GdtArray to hold all base of Gdt Table for all processors. > + GdtBase for processor X is: GdtArray[X] > **/ > -VOID * > +UINTN * > InitGdt ( > - IN UINTN Cr3, > - OUT UINTN *GdtStepSize > + IN UINTN Cr3 > ) > { > UINTN Index; > @@ -59,6 +57,10 @@ InitGdt ( > UINTN GdtTssTableSize; > UINT8 *GdtTssTables; > UINTN GdtTableStepSize; > + UINTN *GdtArray; > + > + GdtArray = AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus); > + ASSERT (GdtArray != NULL); > > // > // For X64 SMM, we allocate separate GDT/TSS for each CPUs to avoid TSS load contention > @@ -89,10 +91,58 @@ InitGdt ( > // > *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase + EFI_PAGE_SIZE + Index * mSmmStackSize); > } > + GdtArray[Index] = (UINTN)GdtTssTables + GdtTableStepSize * Index; > } > > - *GdtStepSize = GdtTableStepSize; > - return GdtTssTables; > + return GdtArray; > +} > + > +/** > + 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); > + 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 > + ); > } > > /** > @@ -154,4 +204,3 @@ TransferApToSafeState ( > ASSERT (FALSE); > } > > - > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org> https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-11-24 0:46 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-23 14:54 [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault Jiewen Yao
2016-11-23 22:28 ` Laszlo Ersek
[not found] ` <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com>
2016-11-24 0:45 ` Yao, Jiewen
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox