1. Patchew
  2. EDK2
  3. [edk2-devel] [PATCH v6 0/3] Fix CET shadow stack token busy bit clear issue
  4. View patch

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

Sheng Wei posted 3 patches 4 years, 11 months ago
[edk2-devel] [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not correct
Posted by Sheng Wei 4 years, 11 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72229): https://edk2.groups.io/g/devel/message/72229
Mute This Topic: https://groups.io/mt/80922789/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not correct
Posted by Yao, Jiewen 4 years, 11 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72231): https://edk2.groups.io/g/devel/message/72231
Mute This Topic: https://groups.io/mt/80922789/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Re: [edk2-devel] [PATCH v6 3/3] UefiCpuPkg/PiSmmCpuDxeSmm: Fix SMM stack offset is not correct
Posted by Ni, Ray 4 years, 11 months ago 
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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72256): https://edk2.groups.io/g/devel/message/72256
Mute This Topic: https://groups.io/mt/80922789/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.