From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.115; helo=mga14.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 08E5C2220D204 for ; Thu, 11 Jan 2018 23:54:04 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Jan 2018 23:59:19 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,348,1511856000"; d="scan'208";a="9577927" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga002.fm.intel.com with ESMTP; 11 Jan 2018 23:59:18 -0800 Received: from fmsmsx125.amr.corp.intel.com (10.18.125.40) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 11 Jan 2018 23:59:18 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX125.amr.corp.intel.com (10.18.125.40) with Microsoft SMTP Server (TLS) id 14.3.319.2; Thu, 11 Jan 2018 23:59:18 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.189]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.213]) with mapi id 14.03.0319.002; Fri, 12 Jan 2018 15:59:16 +0800 From: "Yao, Jiewen" To: "Zeng, Star" , "edk2-devel@lists.01.org" CC: "Wang, Jian J" , "Dong, Eric" , Laszlo Ersek Thread-Topic: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 Thread-Index: AQHTi3c+ILbJa7ay90q/0ePcmh/dzqNv3vmw Date: Fri, 12 Jan 2018 07:59:16 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AA7B054@shsmsx102.ccr.corp.intel.com> References: <1515742219-11952-1-git-send-email-star.zeng@intel.com> In-Reply-To: <1515742219-11952-1-git-send-email-star.zeng@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYzE4OWFlYzAtZTMzMS00MDNmLThkYjMtNmQyZjVmMzIyNGMzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjIuNS4xOCIsIlRydXN0ZWRMYWJlbEhhc2giOiJXY0JUSmtMYXNaQXpIYTlHT1dWeXRkMVwvUjk5bmVjTkh5anpra215T2Z2T0JQeEExK0pjTnpHOGNGaWRlN2VGbCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.0.116 dlp-reaction: no-action 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:05 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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); > + 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 ; > Wang, Jian J ; Dong, Eric ; L= aszlo > Ersek > Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page > fault for IA32 >=20 > When StackGuard is enabled on IA32, the #double fault exception > is 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 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 > write GDT and TSS, so AllocateCodePages() could not be used. >=20 > This patch uses AllocatePages() instead of AllocateCodePages() to > 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 reserved.
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e found > at > @@ -77,7 +77,12 @@ InitGdt ( >=20 > GdtTssTableSize =3D (gcSmiGdtr.Limit + 1 + TSS_SIZE * 2 + 7) & ~7; /= / 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 > /** > - 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 reserved.
> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.
> Copyright (c) 2017, AMD Incorporated. All rights reserved.
>=20 > This program and the accompanying materials > @@ -510,14 +510,6 @@ 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 reserved.
> +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e 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 should have been set to RO > + // if it is allocated with 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 reserved.
> +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.
> This program and the accompanying materials > are licensed and made available under the terms and conditions of the BS= D > License > which accompanies this distribution. The full text of the license may b= e 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