Re: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page fault for IA32

["Yao, Jiewen" <jiewen.yao@intel.com>]

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