Thanks to fix this.
The update seems good. Reviewed-by: jiewen....@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.z...@intel.com>; Yao, Jiewen <jiewen....@intel.com>;
> Wang, Jian J <jian.j.w...@intel.com>; Dong, Eric <eric.d...@intel.com>; Laszlo
> Ersek <ler...@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....@intel.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.z...@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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
