From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) (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 3382E81F30 for ; Tue, 6 Dec 2016 17:51:35 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga104.jf.intel.com with ESMTP; 06 Dec 2016 17:51:34 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,310,1477983600"; d="scan'208";a="39734483" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga005.fm.intel.com with ESMTP; 06 Dec 2016 17:51:34 -0800 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 6 Dec 2016 17:51:34 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.248.2; Tue, 6 Dec 2016 17:51:34 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.239]) with mapi id 14.03.0248.002; Wed, 7 Dec 2016 09:51:32 +0800 From: "Fan, Jeff" To: "Yao, Jiewen" , "edk2-devel@lists.01.org" CC: Laszlo Ersek , "Kinney, Michael D" Thread-Topic: [PATCH V2] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault. Thread-Index: AQHSS8sVotv5sjswyUeGKGieMawdmaD7wOeg Date: Wed, 7 Dec 2016 01:51:31 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2F57A0@shsmsx102.ccr.corp.intel.com> References: <1480593847-3880-1-git-send-email-jiewen.yao@intel.com> In-Reply-To: <1480593847-3880-1-git-send-email-jiewen.yao@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNGQ4YTJlNjAtMTQxNi00ZGQ1LWE2MTgtYzk0YWI0NTUzZGYzIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6Im16VmhQNitSYWJYd3BZeldqRUJnWDJscFVaT3lcL2NDN0tDRFZmNExmc1IwPSJ9 x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH V2] 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: Wed, 07 Dec 2016 01:51:35 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Jeff Fan -----Original Message----- From: Yao, Jiewen=20 Sent: Thursday, December 01, 2016 8:04 PM To: edk2-devel@lists.01.org Cc: Laszlo Ersek; Fan, Jeff; Kinney, Michael D Subject: [PATCH V2] UefiCpuPkg/PiSmmCpu: Fixed #double fault on #page fault= . This patch fixes https://bugzilla.tianocore.org/show_bug.cgi?id=3D246 Previously, when SMM exception happens after EndOfDxe, with StackGuard enab= led 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 happen= s. We decide to not lock GDT for IA32 StackGuard enabled. This issue does not exist on X64, or IA32 without StackGuard. 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/SmmFuncsArch.c | 55 ++++++++++++++++ UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 68 ++++++++++++++++++= ++ UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c | 48 -------------- UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 49 +++++++++++++- 4 files changed, 171 insertions(+), 49 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c b/UefiCpuPkg/PiS= mmCpuDxeSmm/Ia32/SmmFuncsArch.c index f4db6c8..3c68c97 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmFuncsArch.c @@ -127,6 +127,61 @@ 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=20 + (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 S= 3 patch. =20 @param[in] ApHltLoopCode The address of the safe hlt-loop funct= ion. diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSmmC= puDxeSmm/PiSmmCpuDxeSmm.h index abe5cc6..c415858 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h @@ -524,6 +524,14 @@ InitGdt ( ); =20 /** + This function sets GDT/IDT buffer to be RO and XP. +**/ +VOID +PatchGdtIdtMap ( + VOID + ); + +/** =20 Register the SMM Foundation entry point. =20 @@ -596,6 +604,66 @@ SmmBlockingStartupThisAp ( ); =20 /** + This function sets the attributes for the memory region specified by=20 + BaseAddress and Length from their current attributes to the attributes s= pecified by Attributes. + + @param[in] BaseAddress The physical address that is the start addr= ess 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 m= emory region. + + @retval EFI_SUCCESS The attributes were set for the memory reg= ion. + @retval EFI_ACCESS_DENIED The attributes for the memory resource ran= ge specified by + BaseAddress and Length cannot be modified. + @retval EFI_INVALID_PARAMETER Length is zero. + Attributes specified an illegal combinatio= n of attributes that + cannot be set together. + @retval EFI_OUT_OF_RESOURCES There are not enough system resources to m= odify 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 an= d 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=20 +by BaseAddress and + Length from their current attributes to the attributes specified by Attr= ibutes. + + @param[in] BaseAddress The physical address that is the start addr= ess 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 ran= ge specified by + BaseAddress and Length cannot be modified. + @retval EFI_INVALID_PARAMETER Length is zero. + Attributes specified an illegal combinatio= n of attributes that + cannot be set together. + @retval EFI_OUT_OF_RESOURCES There are not enough system resources to m= odify 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 an= d 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. =20 **/ diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c b/UefiCpuPk= g/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c index c85e025..fc440e4 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmCpuMemoryManagement.c @@ -750,54 +750,6 @@ 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); - 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/PiSm= mCpuDxeSmm/X64/SmmFuncsArch.c index 9fc00c1..9d26e44 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c @@ -96,6 +96,54 @@ 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= =20 + ( + 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. @@ -154,4 +202,3 @@ TransferApToSafeState ( ASSERT (FALSE); } =20 - -- 2.7.4.windows.1