From: Laszlo Ersek <lersek@redhat.com>
To: Jiewen Yao <jiewen.yao@intel.com>, edk2-devel@ml01.01.org
Cc: Michael D Kinney <michael.d.kinney@intel.com>,
Jeff Fan <jeff.fan@intel.com>
Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault.
Date: Wed, 23 Nov 2016 23:28:34 +0100 [thread overview]
Message-ID: <bd823dfa-ce4b-4963-bc3a-7021e324dfb5@redhat.com> (raw)
In-Reply-To: <1479912886-38896-1-git-send-email-jiewen.yao@intel.com>
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);
> }
>
> -
>
next prev parent reply other threads:[~2016-11-23 22:28 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-11-23 14:54 [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault Jiewen Yao
2016-11-23 22:28 ` Laszlo Ersek [this message]
[not found] ` <ED077930C258884BBCB450DB737E6622486AB46E@shsmsx102.ccr.corp.intel.com>
2016-11-24 0:45 ` Yao, Jiewen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bd823dfa-ce4b-4963-bc3a-7021e324dfb5@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox