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

["Ni, Ray" <ray.ni@intel.com>]

Reviewed-by: Ray Ni <ray.ni@intel.com>

> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: Friday, February 26, 2021 8:22 PM
> To: Sheng, W <w.sheng@intel.com>; 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>;
> Feng, Roger <roger.feng@intel.com>
> Subject: RE: [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack
> offset is not correct
> 
> 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