Re: [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not correct

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

Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Sheng, W <w.sheng@intel.com>
> Sent: Friday, February 26, 2021 4:03 PM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Feng, Roger <roger.feng@intel.com>
> Subject: [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is
> not correct
> 
> In function InitGdt(), SmiPFHandler() and Gen4GPageTable(), it uses
>  CpuIndex * mSmmStackSize to get the SMM stack address offset for
>  multi processor. It misses the SMM Shadow Stack Size. Each processor
>  will use mSmmStackSize + mSmmShadowStackSize in the memory.
> It should use CpuIndex * (mSmmStackSize + mSmmShadowStackSize) to get
>  this SMM stack address offset. If mSmmShadowStackSize > 0 and multi
>  processor enabled, it will get the wrong offset value.
> CET shadow stack feature will set the value of mSmmShadowStackSize.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3237
> 
> Signed-off-by: Sheng Wei <w.sheng@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Rahul Kumar <rahul1.kumar@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Roger Feng <roger.feng@intel.com>
> ---
>  UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c        | 6 ++++--
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c      | 4 +++-
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 2 +-
>  3 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> index 4bcd217917..6227b2428a 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c
> @@ -23,6 +23,8 @@ SPIN_LOCK                                   *mPFLock = NULL;
>  SMM_CPU_SYNC_MODE                           mCpuSmmSyncMode;
>  BOOLEAN                                     mMachineCheckSupported = FALSE;
> 
> +extern UINTN mSmmShadowStackSize;
> +
>  /**
>    Performs an atomic compare exchange operation to get semaphore.
>    The compare exchange operation must be performed using
> @@ -920,7 +922,7 @@ Gen4GPageTable (
>      // Add two more pages for known good stack and stack guard page,
>      // then find the lower 2MB aligned address.
>      //
> -    High2MBoundary = (mSmmStackArrayEnd - mSmmStackSize + EFI_PAGE_SIZE
> * 2) & ~(SIZE_2MB-1);
> +    High2MBoundary = (mSmmStackArrayEnd - mSmmStackSize -
> mSmmShadowStackSize + EFI_PAGE_SIZE * 2) & ~(SIZE_2MB-1);
>      PagesNeeded = ((High2MBoundary - Low2MBoundary) / SIZE_2MB) + 1;
>    }
>    //
> @@ -971,7 +973,7 @@ Gen4GPageTable (
>            // Mark the guard page as non-present
>            //
>            Pte[Index] = PageAddress | mAddressEncMask;
> -          GuardPage += mSmmStackSize;
> +          GuardPage += (mSmmStackSize + mSmmShadowStackSize);
>            if (GuardPage > mSmmStackArrayEnd) {
>              GuardPage = 0;
>            }
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> index cdc1fcefc5..07e7ea70de 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/PageTbl.c
> @@ -13,6 +13,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #define PAGE_TABLE_PAGES            8
>  #define ACC_MAX_BIT                 BIT3
> 
> +extern UINTN mSmmShadowStackSize;
> +
>  LIST_ENTRY                          mPagePool = INITIALIZE_LIST_HEAD_VARIABLE
> (mPagePool);
>  BOOLEAN                             m1GPageTableSupport = FALSE;
>  BOOLEAN                             mCpuSmmRestrictedMemoryAccess;
> @@ -1037,7 +1039,7 @@ SmiPFHandler (
>        (PFAddress < (mCpuHotPlugData.SmrrBase + mCpuHotPlugData.SmrrSize))) {
>      DumpCpuContext (InterruptType, SystemContext);
>      CpuIndex = GetCpuIndex ();
> -    GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex *
> mSmmStackSize);
> +    GuardPageAddress = (mSmmStackArrayBase + EFI_PAGE_SIZE + CpuIndex *
> (mSmmStackSize + mSmmShadowStackSize));
>      if ((FeaturePcdGet (PcdCpuSmmStackGuard)) &&
>          (PFAddress >= GuardPageAddress) &&
>          (PFAddress < (GuardPageAddress + EFI_PAGE_SIZE))) {
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> index 7ef3b1d488..661c1ba294 100644
> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> @@ -93,7 +93,7 @@ InitGdt (
>        //
>        // Setup top of known good stack as IST1 for each processor.
>        //
> -      *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase +
> EFI_PAGE_SIZE + Index * mSmmStackSize);
> +      *(UINTN *)(TssBase + TSS_X64_IST1_OFFSET) = (mSmmStackArrayBase +
> EFI_PAGE_SIZE + Index * (mSmmStackSize + mSmmShadowStackSize));
>      }
>    }
> 
> --
> 2.16.2.windows.1