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

2018-01-12 Thread 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 ; edk2-devel@lists.01.org
Cc: Wang, Jian J ; Dong, Eric ; 
Laszlo Ersek 
Subject: RE: [PATCH] UefiCpuPkg PiSmmCpuDxeSmm: Fixed #double fault on #page 
fault for IA32

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 ; Yao, Jiewen 
> ; Wang, Jian J ; Dong, 
> Eric ; Laszlo Ersek 
> 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 
> 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(-)
> 
> 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.
> +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 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

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

2018-01-12 Thread Yao, Jiewen 
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 ; Yao, Jiewen ;
> Wang, Jian J ; Dong, Eric ; Laszlo
> Ersek 
> 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 
> 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(-)
> 
> 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.
> +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 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]