* [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 @ 2018-01-12 7:30 Star Zeng 2018-01-12 7:59 ` Yao, Jiewen 0 siblings, 1 reply; 3+ messages in thread From: Star Zeng @ 2018-01-12 7:30 UTC (permalink / raw) To: edk2-devel; +Cc: Star Zeng, Jiewen Yao, Jian J Wang, Eric Dong, Laszlo Ersek When StackGuard is enabled on IA32, the #double fault exception is reported instead of #page fault. This issue does not exist on X64, or IA32 without StackGuard. The fix at e4435f710cea2d2f10cd7343d545920867780086 was incomplete. 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. This patch uses AllocatePages() instead of AllocateCodePages() to allocate buffer for GDT and TSS if StackGuard is enabled on IA32. Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Jian J Wang <jian.j.wang@intel.com> Cc: Eric Dong <eric.dong@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Star Zeng <star.zeng@intel.com> --- 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(-) 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. -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -77,7 +77,12 @@ InitGdt ( 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)); + // + // 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 = (UINT8*)AllocatePages (EFI_SIZE_TO_PAGES (mGdtBufferSize)); ASSERT (GdtTssTables != NULL); mGdtBuffer = (UINTN)GdtTssTables; GdtTableStepSize = GdtTssTableSize; @@ -127,61 +132,6 @@ InitGdt ( } /** - 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); - 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 = gcSmiIdtr.Base; - Size = 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. @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 CPU. -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR> Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> This program and the accompanying materials @@ -510,14 +510,6 @@ InitGdt ( ); /** - This function sets GDT/IDT buffer to be RO and XP. -**/ -VOID -PatchGdtIdtMap ( - VOID - ); - -/** Register the SMM Foundation entry point. 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 -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -769,6 +769,53 @@ 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); + // + // 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 = 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 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. -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR> This program and the accompanying materials are licensed and made available under the terms and conditions of the BSD License which accompanies this distribution. The full text of the license may be found at @@ -96,54 +96,6 @@ InitGdt ( } /** - 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 - ); -} - -/** Get Protected mode code segment from current GDT table. @return Protected mode code segment value. -- 2.7.0.windows.1 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 2018-01-12 7:30 [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 Star Zeng @ 2018-01-12 7:59 ` Yao, Jiewen 2018-01-12 8:00 ` Zeng, Star 0 siblings, 1 reply; 3+ messages in thread From: Yao, Jiewen @ 2018-01-12 7:59 UTC (permalink / raw) To: Zeng, Star, edk2-devel@lists.01.org Cc: Wang, Jian J, Dong, Eric, Laszlo Ersek 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 = (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 = gcSmiIdtr.Base; > + Size = 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 <star.zeng@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; > Wang, Jian J <jian.j.wang@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo > Ersek <lersek@redhat.com> > Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page > fault for IA32 > > When StackGuard is enabled on IA32, the #double fault exception > is reported instead of #page fault. > > This issue does not exist on X64, or IA32 without StackGuard. > > The fix at e4435f710cea2d2f10cd7343d545920867780086 was incomplete. > > 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. > > This patch uses AllocatePages() instead of AllocateCodePages() to > allocate buffer for GDT and TSS if StackGuard is enabled on IA32. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng <star.zeng@intel.com> > --- > 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(-) > > 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. > > -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be found > at > @@ -77,7 +77,12 @@ InitGdt ( > > 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)); > + // > + // 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 = (UINT8*)AllocatePages (EFI_SIZE_TO_PAGES > (mGdtBufferSize)); > ASSERT (GdtTssTables != NULL); > mGdtBuffer = (UINTN)GdtTssTables; > GdtTableStepSize = GdtTssTableSize; > @@ -127,61 +132,6 @@ InitGdt ( > } > > /** > - 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); > - 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 = gcSmiIdtr.Base; > - Size = 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. > > @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 CPU. > > -Copyright (c) 2009 - 2017, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR> > Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> > > This program and the accompanying materials > @@ -510,14 +510,6 @@ InitGdt ( > ); > > /** > - This function sets GDT/IDT buffer to be RO and XP. > -**/ > -VOID > -PatchGdtIdtMap ( > - VOID > - ); > - > -/** > > Register the SMM Foundation entry point. > > 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 > > -Copyright (c) 2016 - 2017, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be found > at > @@ -769,6 +769,53 @@ 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); > + // > + // 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 = 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 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. > > -Copyright (c) 2015 - 2016, Intel Corporation. All rights reserved.<BR> > +Copyright (c) 2015 - 2018, Intel Corporation. All rights reserved.<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license may be found > at > @@ -96,54 +96,6 @@ InitGdt ( > } > > /** > - 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 > - ); > -} > - > -/** > Get Protected mode code segment from current GDT table. > > @return Protected mode code segment value. > -- > 2.7.0.windows.1 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 2018-01-12 7:59 ` Yao, Jiewen @ 2018-01-12 8:00 ` Zeng, Star 0 siblings, 0 replies; 3+ messages in thread From: Zeng, Star @ 2018-01-12 8:00 UTC (permalink / raw) To: Yao, Jiewen, edk2-devel@lists.01.org Cc: Wang, Jian J, Dong, Eric, Laszlo Ersek, Zeng, Star Yes, right. Patch for that case is on the way. Thanks, Star -----Original Message----- From: Yao, Jiewen Sent: Friday, January 12, 2018 3:59 PM To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org Cc: Wang, Jian J <jian.j.wang@intel.com>; Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> Subject: RE: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page 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 = (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 = gcSmiIdtr.Base; > + Size = 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 <star.zeng@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Wang, Jian J <jian.j.wang@intel.com>; Dong, > Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on > #page fault for IA32 > > When StackGuard is enabled on IA32, the #double fault exception is > reported instead of #page fault. > > This issue does not exist on X64, or IA32 without StackGuard. > > The fix at e4435f710cea2d2f10cd7343d545920867780086 was incomplete. > > 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. > > This patch uses AllocatePages() instead of AllocateCodePages() to > allocate buffer for GDT and TSS if StackGuard is enabled on IA32. > > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Jian J Wang <jian.j.wang@intel.com> > Cc: Eric Dong <eric.dong@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Star Zeng <star.zeng@intel.com> > --- > 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(-) > > 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. > > -Copyright (c) 2015 - 2016, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2015 - 2018, Intel Corporation. All rights > +reserved.<BR> > This program and the accompanying materials are licensed and made > available under the terms and conditions of the BSD License which > accompanies this distribution. The full text of the license may be > found at @@ -77,7 +77,12 @@ InitGdt ( > > 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)); > + // > + // 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 = (UINT8*)AllocatePages (EFI_SIZE_TO_PAGES > (mGdtBufferSize)); > ASSERT (GdtTssTables != NULL); > mGdtBuffer = (UINTN)GdtTssTables; > GdtTableStepSize = GdtTssTableSize; @@ -127,61 +132,6 @@ InitGdt > ( } > > /** > - 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); > - 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 = gcSmiIdtr.Base; > - Size = 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. > > @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 CPU. > > -Copyright (c) 2009 - 2017, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2009 - 2018, Intel Corporation. All rights > +reserved.<BR> > Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR> > > This program and the accompanying materials @@ -510,14 +510,6 @@ > InitGdt ( > ); > > /** > - This function sets GDT/IDT buffer to be RO and XP. > -**/ > -VOID > -PatchGdtIdtMap ( > - VOID > - ); > - > -/** > > Register the SMM Foundation entry point. > > 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 > > -Copyright (c) 2016 - 2017, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2016 - 2018, Intel Corporation. All rights > +reserved.<BR> > This program and the accompanying materials are licensed and made > available under the terms and conditions of the BSD License which > accompanies this distribution. The full text of the license may be > found at @@ -769,6 +769,53 @@ 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); // // 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 = 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 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. > > -Copyright (c) 2015 - 2016, Intel Corporation. All rights > reserved.<BR> > +Copyright (c) 2015 - 2018, Intel Corporation. All rights > +reserved.<BR> > This program and the accompanying materials are licensed and made > available under the terms and conditions of the BSD License which > accompanies this distribution. The full text of the license may be > found at @@ -96,54 +96,6 @@ InitGdt ( } > > /** > - 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 > - ); > -} > - > -/** > Get Protected mode code segment from current GDT table. > > @return Protected mode code segment value. > -- > 2.7.0.windows.1 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-01-12 7:54 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-12 7:30 [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32 Star Zeng 2018-01-12 7:59 ` Yao, Jiewen 2018-01-12 8:00 ` Zeng, Star
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox