From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.20; helo=mga02.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 8FCA32220D204 for ; Thu, 11 Jan 2018 23:54:57 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jan 2018 00:00:11 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,348,1511856000"; d="scan'208";a="21021432" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga001.fm.intel.com with ESMTP; 12 Jan 2018 00:00:11 -0800 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 12 Jan 2018 00:00:11 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.319.2; Fri, 12 Jan 2018 00:00:10 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.159]) with mapi id 14.03.0319.002; Fri, 12 Jan 2018 16:00:09 +0800 From: "Zeng, Star" To: "Yao, Jiewen" , "edk2-devel@lists.01.org" CC: "Wang, Jian J" , "Dong, Eric" , Laszlo Ersek , "Zeng, Star" Thread-Topic: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 Thread-Index: AQHTi3s+9c10nJn8QkWl0FcAXUSXZqNv36Iw Date: Fri, 12 Jan 2018 08:00:08 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB0483103B9FBB23@shsmsx102.ccr.corp.intel.com> References: <1515742219-11952-1-git-send-email-star.zeng@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AA7B054@shsmsx102.ccr.corp.intel.com> In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503AA7B054@shsmsx102.ccr.corp.intel.com> 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 Subject: Re: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 12 Jan 2018 07:54:57 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Yes, right. Patch for that case is on the way. Thanks, Star -----Original Message----- From: Yao, Jiewen=20 Sent: Friday, January 12, 2018 3:59 PM To: Zeng, Star ; edk2-devel@lists.01.org Cc: Wang, Jian J ; Dong, Eric ;= Laszlo Ersek Subject: RE: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #pag= e fault for IA32 Thanks to fix this. The update seems good. Reviewed-by: Jiewen.yao@intel.com I reviewed more code and find we already set IDT to be RO. gcSmiIdtr.Base =3D (UINTN)AllocateCodePages (EFI_SIZE_TO_PAGES(gcSmiIdtr.= Limit + 1)); So we do not need set RO for IDT as well. (Setting XP is still needed). > + BaseAddress =3D gcSmiIdtr.Base; > + Size =3D ALIGN_VALUE(gcSmiIdtr.Limit + 1, SIZE_4KB); =20 > + SmmSetMemoryAttributes ( > + BaseAddress, > + Size, > + EFI_MEMORY_RO > + ); Thank you Yao Jiewen > -----Original Message----- > From: Zeng, Star > Sent: Friday, January 12, 2018 3:30 PM > To: edk2-devel@lists.01.org > Cc: Zeng, Star ; Yao, Jiewen=20 > ; Wang, Jian J ; Dong,=20 > Eric ; Laszlo Ersek > Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on=20 > #page fault for IA32 >=20 > When StackGuard is enabled on IA32, the #double fault exception is=20 > reported instead of #page fault. >=20 > This issue does not exist on X64, or IA32 without StackGuard. >=20 > The fix at e4435f710cea2d2f10cd7343d545920867780086 was incomplete. >=20 > It is because AllocateCodePages() is used to allocate buffer for GDT=20 > and TSS, the code pages will be set to RO in SetMemMapAttributes(). > But IA32 Stack Guard need use task switch to switch stack that need=20 > write GDT and TSS, so AllocateCodePages() could not be used. >=20 > This patch uses AllocatePages() instead of AllocateCodePages() to=20 > allocate buffer for GDT and TSS if StackGuard is enabled on IA32. >=20 > Cc: Jiewen Yao > Cc: Jian J Wang > Cc: Eric Dong > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng > --- > UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c | 64 > +++------------------- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 10 +--- > UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 49 > ++++++++++++++++- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 50 > +---------------- > 4 files changed, 57 insertions(+), 116 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > index 3c68c970245f..4c1499939b1b 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c > @@ -1,7 +1,7 @@ > /** @file > SMM CPU misc functions for Ia32 arch specific. >=20 > -Copyright (c) 2015 - 2016, Intel Corporation. All rights=20 > reserved.
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights=20 > +reserved.
> This program and the accompanying materials are licensed and made=20 > available under the terms and conditions of the BSD License which=20 > accompanies this distribution. The full text of the license may be=20 > found at @@ -77,7 +77,12 @@ InitGdt ( >=20 > GdtTssTableSize =3D (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7;=20 > // 8 bytes aligned > mGdtBufferSize =3D GdtTssTableSize * > gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > - GdtTssTables =3D (UINT8*)AllocateCodePages (EFI_SIZE_TO_PAGES > (mGdtBufferSize)); > + // > + // IA32 Stack Guard need use task switch to switch stack that need > + // write GDT and TSS, so AllocateCodePages() could not be used here > + // as code pages will be set to RO. > + // > + GdtTssTables =3D (UINT8*)AllocatePages (EFI_SIZE_TO_PAGES > (mGdtBufferSize)); > ASSERT (GdtTssTables !=3D NULL); > mGdtBuffer =3D (UINTN)GdtTssTables; > GdtTableStepSize =3D GdtTssTableSize; @@ -127,61 +132,6 @@ InitGdt=20 > ( } >=20 > /** > - 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); > - if (!FeaturePcdGet (PcdCpuSmmStackGuard)) { > - // > - // Do not set RO for IA32 when stack guard feature is enabled. > - // Stack Guard need use task switch to switch stack. > - // It need write GDT and TSS. > - // > - 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 > - ); > -} > - > -/** > Transfer AP to safe hlt-loop after it finished restore CPU features on= S3 patch. >=20 > @param[in] ApHltLoopCode The address of the safe hlt-loop > function. > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index ef32f1767665..cbaa513244d5 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1,7 +1,7 @@ > /** @file > Agent Module to load other modules to deploy SMM Entry Vector for X86 CP= U. >=20 > -Copyright (c) 2009 - 2017, Intel Corporation. All rights=20 > reserved.
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights=20 > +reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
>=20 > This program and the accompanying materials @@ -510,14 +510,6 @@=20 > InitGdt ( > ); >=20 > /** > - This function sets GDT/IDT buffer to be RO and XP. > -**/ > -VOID > -PatchGdtIdtMap ( > - VOID > - ); > - > -/** >=20 > Register the SMM Foundation entry point. >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > index 2d7dba59bf30..16664f304cde 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c > @@ -1,6 +1,6 @@ > /** @file >=20 > -Copyright (c) 2016 - 2017, Intel Corporation. All rights=20 > reserved.
> +Copyright (c) 2016 - 2018, Intel Corporation. All rights=20 > +reserved.
> This program and the accompanying materials are licensed and made=20 > available under the terms and conditions of the BSD License which=20 > accompanies this distribution. The full text of the license may be=20 > found at @@ -769,6 +769,53 @@ PatchSmmSaveStateMap ( } >=20 > /** > + 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); // // The range=20 > + should have been set to RO // if it is allocated with=20 > + EfiRuntimeServicesCode. > + // > + 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/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > index 9d26e44a9acd..6a5d453242ff 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c > @@ -1,7 +1,7 @@ > /** @file > SMM CPU misc functions for x64 arch specific. >=20 > -Copyright (c) 2015 - 2016, Intel Corporation. All rights=20 > reserved.
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights=20 > +reserved.
> This program and the accompanying materials are licensed and made=20 > available under the terms and conditions of the BSD License which=20 > accompanies this distribution. The full text of the license may be=20 > found at @@ -96,54 +96,6 @@ InitGdt ( } >=20 > /** > - 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 > - ); > -} > - > -/** > Get Protected mode code segment from current GDT table. >=20 > @return Protected mode code segment value. > -- > 2.7.0.windows.1