From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id B086D81ED1 for ; Wed, 23 Nov 2016 16:46:12 -0800 (PST) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 23 Nov 2016 16:46:12 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,689,1473145200"; d="scan'208,217";a="8676071" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by orsmga002.jf.intel.com with ESMTP; 23 Nov 2016 16:46:11 -0800 Received: from fmsmsx120.amr.corp.intel.com (10.18.124.208) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 16:46:11 -0800 Received: from shsmsx104.ccr.corp.intel.com (10.239.4.70) by fmsmsx120.amr.corp.intel.com (10.18.124.208) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 16:46:10 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX104.ccr.corp.intel.com ([169.254.5.142]) with mapi id 14.03.0248.002; Thu, 24 Nov 2016 08:45:50 +0800 From: "Yao, Jiewen" To: "edk2-devel@ml01.01.org" , "Laszlo Ersek (lersek@redhat.com)" CC: "Kinney, Michael D" , "Fan, Jeff" Thread-Topic: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. Thread-Index: AQHSRZma9VXHMztgSEKjRCF2Na5ePqDmoIoAgAAkhoCAAIZ60A== Date: Thu, 24 Nov 2016 00:45:49 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C50386D9CED@shsmsx102.ccr.corp.intel.com> References: <1479912886-38896-1-git-send-email-jiewen.yao@intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 X-Content-Filtered-By: Mailman/MimeDel 2.1.21 Subject: Re: [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Nov 2016 00:46:12 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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 i= f there is other concern. Thank you Yao Jiewen -----Original Message----- From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Lasz= lo Ersek Sent: Thursday, November 24, 2016 6:29 AM To: Yao, Jiewen; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Fan, Jeff Subject: Re: [edk2] [PATCH] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #pa= ge fault. On 11/23/16 15:54, Jiewen Yao wrote: > This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3D246 > > 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 switc= h. > 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 > > Cc: Jeff Fan > > Cc: Michael D Kinney > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jiewen Yao > > --- > 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 > 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 . 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: . For that, Regression-tested-by: Laszlo Ersek > - 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/PiSmm= CpuDxeSmm/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/PiS= mmCpuDxeSmm/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/Pi= SmmCpuDxeSmm/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/P= iSmmCpuDxeSmm/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 =3D offset CodeSeg32 - offset NullSeg > DATA_SEL =3D offset DataSeg32 - offset NullSeg > -TSS_SEL =3D offset TssSeg - offset NullSeg > -EXCEPTION_TSS_SEL =3D 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/UefiCpuPk= g/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/P= iSmmCpuDxeSmm/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 =3D AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntr= yContext.NumberOfCpus); > + ASSERT (GdtArray !=3D 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 --= ------------>| > + // | | = | > + // +--------------------------------+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D+--------------+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D+ > + // | GDT(0) | GDT(1) | ... | GDT(n) | GDT_TSS(0) | GDT_TSS(1) | ... = | GDT_TSS(n) | GDT(n+1) |...| GDT_TSS(n+1) |...| > + // +--------------------------------+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D+--------------+=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D+ > + // | | | > + // |<------------ Gdt.Limit ------------>|<-TSS->| > // > - gcSmiGdtr.Limit +=3D (UINT16)(2 * sizeof (IA32_SEGMENT_DESCRIPTOR)); > > - GdtTssTableSize =3D (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; /= / 8 bytes aligned > - mGdtBufferSize =3D GdtTssTableSize * gSmmCpuPrivate->SmmCoreEntryCon= text.NumberOfCpus; > - GdtTssTables =3D (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES (mGdtB= ufferSize)); > + GdtTableStepSize =3D (2 * sizeof (IA32_SEGMENT_DESCRIPTOR) + TSS_SIZ= E * 2 + 7) & ~7; // 8 bytes aligned > + if (GdtTableStepSize < (UINTN)(gcSmiGdtr.Limit + 1)) { > + GdtTableStepSize =3D gcSmiGdtr.Limit + 1; > + } > + GdtCountPerPage =3D EFI_PAGE_SIZE / GdtTableStepSize; > + BundleCount =3D (gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus + = GdtCountPerPage - 1) / GdtCountPerPage; > + mGdtBufferSize =3D EFI_PAGES_TO_SIZE(BundleCount * 2); > + > + GdtTssTables =3D (UINT8*)AllocateCodePages (BundleCount * 2); > ASSERT (GdtTssTables !=3D NULL); > + ZeroMem ((VOID *)GdtTssTables, EFI_PAGES_TO_SIZE(BundleCount * 2)); > mGdtBuffer =3D (UINTN)GdtTssTables; > - GdtTableStepSize =3D GdtTssTableSize; > > - for (Index =3D 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.Number= OfCpus; Index++) { > - CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gc= SmiGdtr.Base, gcSmiGdtr.Limit + 1 + TSS_SIZE * 2); > - // > - // Fixup TSS descriptors > - // > - TssBase =3D (UINTN)(GdtTssTables + GdtTableStepSize * Index + gcSm= iGdtr.Limit + 1); > - GdtDescriptor =3D (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > - GdtDescriptor->Bits.BaseLow =3D (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid =3D (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh =3D (UINT8)(TssBase >> 24); > - > - TssBase +=3D TSS_SIZE; > - GdtDescriptor++; > - GdtDescriptor->Bits.BaseLow =3D (UINT16)TssBase; > - GdtDescriptor->Bits.BaseMid =3D (UINT8)(TssBase >> 16); > - GdtDescriptor->Bits.BaseHigh =3D (UINT8)(TssBase >> 24); > - // > - // Fixup TSS segments > - // > - // ESP as known good stack > - // > - *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) =3D mSmmStackArrayBase = + EFI_PAGE_SIZE + Index * mSmmStackSize; > - *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) =3D Cr3; > + Index =3D 0; > + for (BundleIndex =3D 0; BundleIndex < BundleCount; BundleIndex++) { > + CountMax =3D GdtCountPerPage; > + if (BundleIndex =3D=3D BundleCount - 1) { > + CountMax =3D gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus - = GdtCountPerPage * BundleIndex; > + } > + BundleBase =3D GdtTssTables + EFI_PAGE_SIZE * 2 * BundleIndex; > + for (GdtIndex =3D 0; GdtIndex < CountMax; GdtIndex++, Index++) { > + GdtArray[Index] =3D (UINTN)BundleBase + GdtTableStepSize * GdtIn= dex; > + // > + // 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 =3D (UINTN)(GdtArray[Index] + EFI_PAGE_SIZE + sizeof(IA3= 2_SEGMENT_DESCRIPTOR) * 2); > + CopyMem ( > + (VOID *)(TssBase), > + (VOID*)((UINTN)gcSmiGdtr.Base + gcSmiGdtr.Limit + 1 + sizeof(I= A32_SEGMENT_DESCRIPTOR) * 2), > + TSS_SIZE * 2 > + ); > + // > + // Fixup TSS descriptors > + // > + GdtDescriptor =3D (IA32_SEGMENT_DESCRIPTOR *)(TssBase) - 2; > + GdtDescriptor->Bits.BaseLow =3D (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid =3D (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh =3D (UINT8)(TssBase >> 24); > + > + TssBase +=3D TSS_SIZE; > + GdtDescriptor++; > + GdtDescriptor->Bits.BaseLow =3D (UINT16)TssBase; > + GdtDescriptor->Bits.BaseMid =3D (UINT8)(TssBase >> 16); > + GdtDescriptor->Bits.BaseHigh =3D (UINT8)(TssBase >> 24); > + // > + // Fixup TSS segments > + // > + // ESP as known good stack > + // > + *(UINTN *)(TssBase + TSS_IA32_ESP_OFFSET) =3D mSmmStackArrayBas= e + EFI_PAGE_SIZE + Index * mSmmStackSize; > + *(UINT32 *)(TssBase + TSS_IA32_CR3_OFFSET) =3D Cr3; > + } > } > + // > + // Enlarge GDT to contain 1 page plus 2 TSS descriptors > + // > + gcSmiGdtr.Limit =3D (UINT16)(EFI_PAGE_SIZE + 2 * sizeof (IA32_SEGMEN= T_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 =3D 0; Index < gSmmCpuPrivate->SmmCoreEntryContext.Number= OfCpus; Index++) { > CopyMem (GdtTssTables + GdtTableStepSize * Index, (VOID*)(UINTN)gc= SmiGdtr.Base, gcSmiGdtr.Limit + 1); > + GdtArray[Index] =3D (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 =3D mGdtBuffer; > + Size =3D ALIGN_VALUE(mGdtBufferSize, SIZE_4KB); > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_XP > + ); > + BundleCount =3D EFI_SIZE_TO_PAGES(mGdtBufferSize)/2; > + for (Index =3D 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 =3D mGdtBuffer; > + Size =3D 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 =3D GdtTableStepSize; > - return GdtTssTables; > + BaseAddress =3D gcSmiIdtr.Base; > + Size =3D 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/PiSmmCpuD= xeSmm/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 =3D SmmInitPageTable (); > > - GdtTssTables =3D InitGdt (Cr3, &GdtTableStepSize); > + GdtArray =3D InitGdt (Cr3); > > // > // Initialize PROCESSOR_SMM_DESCRIPTOR for each CPU > @@ -1415,7 +1414,7 @@ InitializeMpServiceData ( > for (Index =3D 0; Index < mMaxNumberOfCpus; Index++) { > Psd =3D (PROCESSOR_SMM_DESCRIPTOR *)(VOID *)(UINTN)(mCpuHotPlugData.= SmBase[Index] + SMM_PSD_OFFSET); > CopyMem (Psd, &gcPsd, sizeof (gcPsd)); > - Psd->SmmGdtPtr =3D (UINT64)(UINTN)(GdtTssTables + GdtTableStepSize *= Index); > + Psd->SmmGdtPtr =3D (UINT64)(UINTN)GdtArray[Index]; > Psd->SmmGdtSize =3D gcSmiGdtr.Limit + 1; > > // > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSm= mCpuDxeSmm/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 B= aseAddress and > + Length from their current attributes to the attributes specified by At= tributes. > + > + @param[in] BaseAddress The physical address that is the start ad= dress 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 r= egion. > + @retval EFI_ACCESS_DENIED The attributes for the memory resource r= ange specified by > + BaseAddress and Length cannot be modifie= d. > + @retval EFI_INVALID_PARAMETER Length is zero. > + Attributes specified an illegal combinat= ion 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 mo= re bytes of the memory > + resource range specified by BaseAddress = and Length. > + The bit mask of attributes is not suppor= t for the memory resource > + range specified by BaseAddress and Lengt= h. > + > +**/ > +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 At= tributes. > + > + @param[in] BaseAddress The physical address that is the start ad= dress 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 t= he memory region. > + > + @retval EFI_SUCCESS The attributes were cleared for the memo= ry region. > + @retval EFI_ACCESS_DENIED The attributes for the memory resource r= ange specified by > + BaseAddress and Length cannot be modifie= d. > + @retval EFI_INVALID_PARAMETER Length is zero. > + Attributes specified an illegal combinat= ion 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 mo= re bytes of the memory > + resource range specified by BaseAddress = and Length. > + The bit mask of attributes is not suppor= t for the memory resource > + range specified by BaseAddress and Lengt= h. > + > +**/ > +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/UefiCpu= Pkg/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 =3D mGdtBuffer; > - Size =3D 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 =3D gcSmiIdtr.Base; > - Size =3D 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/Pi= SmmCpuDxeSmm/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 =3D AllocatePool (sizeof(UINTN) * gSmmCpuPrivate->SmmCoreEntr= yContext.NumberOfCpus); > + ASSERT (GdtArray !=3D NULL); > > // > // For X64 SMM, we allocate separate GDT/TSS for each CPUs to avoid TS= S load contention > @@ -89,10 +91,58 @@ InitGdt ( > // > *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) =3D (mSmmStackArrayBase = + EFI_PAGE_SIZE + Index * mSmmStackSize); > } > + GdtArray[Index] =3D (UINTN)GdtTssTables + GdtTableStepSize * Index; > } > > - *GdtStepSize =3D 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 =3D mGdtBuffer; > + Size =3D 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 =3D gcSmiIdtr.Base; > + Size =3D 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 https://lists.01.org/mailman/listinfo/edk2-devel