Re: [edk2-devel] [PATCH v5 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit

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

Thank you. I have reviewed that.

We still need UefiCpuPkg and MdePkg maintainer's review before merge.

Thank you
Yao Jiewen

> -----Original Message-----
> From: Sheng, W <w.sheng@intel.com>
> Sent: Thursday, February 25, 2021 1:58 PM
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; Sheng, W
> <w.sheng@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> Kumar, Rahul1 <rahul1.kumar@intel.com>; Feng, Roger <roger.feng@intel.com>
> Subject: RE: [edk2-devel] [PATCH v5 2/2] UefiCpuPkg/CpuExceptionHandlerLib:
> Clear CET shadow stack token busy bit
> 
> HI Jiewen,
> I have refined the comment in the code.
> It is working with PcdCpuSmmRestrictedMemoryAccess enabled.
> Add the sample code in file
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDxeSm
> m/X64/SmmFuncsArch.c
> [PATCH v5 1/2] is the patch of add CET instruction DX define in nasm.inc file.
> https://edk2.groups.io/g/devel/message/72182
> 
> Do have any comment for the patch of fix CET shadow stack token busy bit
> issue ?
> Could you give review-by for this patch ?
> 
> Thank you
> BR
> Sheng Wei
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sheng
> > Wei
> > Sent: 2021年2月23日 15:52
> > To: devel@edk2.groups.io; Sheng, W <w.sheng@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> > Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>;
> > Kumar, Rahul1 <rahul1.kumar@intel.com>; Feng, Roger
> > <roger.feng@intel.com>
> > Subject: Re: [edk2-devel] [PATCH v5 2/2]
> > UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit
> >
> > Hi Jiewen, Ray,
> > Could you help to review and give Review-by for this patch for fix CET
> > shadow stack token busy bit issue ?
> > As the common in v5 patch 1/2 by Limin, since it is a bug fix, it can still be
> > merged in 202102 stable tag soft feature freeze phase.
> > https://edk2.groups.io/g/devel/message/72013
> > Thank you.
> > BR
> > Sheng Wei
> >
> > > -----Original Message-----
> > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sheng
> > > Wei
> > > Sent: 2021年2月22日 10:15
> > > To: devel@edk2.groups.io; Sheng, W <w.sheng@intel.com>
> > > 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: Re: [edk2-devel] [PATCH v5 2/2]
> > > UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy
> > > bit
> > >
> > > Hi Jiewen,
> > > Thank you for review the patch.
> > > Could you give review-by on this patch?
> > > Thank you.
> > > BR
> > > Sheng Wei
> > >
> > >
> > > > -----Original Message-----
> > > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
> > Sheng
> > > > Wei
> > > > Sent: 2021年2月20日 11:15
> > > > 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: [edk2-devel] [PATCH v5 2/2]
> > UefiCpuPkg/CpuExceptionHandlerLib:
> > > > Clear CET shadow stack token busy bit
> > > >
> > > > If CET shadows stack feature enabled in SMM and stack switch is enabled.
> > > > When code execute from SMM handler to SMM exception, CPU will
> > check
> > > > SMM exception shadow stack token busy bit if it is cleared or not.
> > > > If it is set, it will trigger #DF exception.
> > > > If it is not set, CPU will set the busy bit when enter SMM exception.
> > > > So, the busy bit should be cleared when return back form SMM
> > > > exception to SMM handler. Otherwise, keeping busy bit 1 will cause
> > > > to trigger #DF exception when enter SMM exception next time.
> > > > So, we use instruction SAVEPREVSSP, CLRSSBSY and RSTORSSP to clear
> > > > the shadow stack token busy bit before RETF instruction in SMM
> > exception.
> > > >
> > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3192
> > > >
> > > > 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>
> > > > ---
> > > >  .../DxeCpuExceptionHandlerLib.inf                  |  3 ++
> > > >  .../PeiCpuExceptionHandlerLib.inf                  |  3 ++
> > > >  .../SecPeiCpuExceptionHandlerLib.inf               |  4 ++
> > > >  .../SmmCpuExceptionHandlerLib.inf                  |  3 ++
> > > >  .../X64/Xcode5ExceptionHandlerAsm.nasm             | 46
> > > > +++++++++++++++++++++-
> > > >  .../Xcode5SecPeiCpuExceptionHandlerLib.inf         |  4 ++
> > > >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c       | 15 ++++++-
> > > >  7 files changed, 75 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.
> > > > inf
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib
> > > > .inf
> > > > index 07b34c92a8..e7a81bebdb 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.
> > > > inf
> > > > +++
> > > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLi
> > > > +++ b.inf
> > > > @@ -43,6 +43,9 @@
> > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuStackSwitchExceptionList
> > > >    gUefiCpuPkgTokenSpaceGuid.PcdCpuKnownGoodStackSize
> > > >
> > > > +[FeaturePcd]
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > > CONSUMES
> > > > +
> > > >  [Packages]
> > > >    MdePkg/MdePkg.dec
> > > >    MdeModulePkg/MdeModulePkg.dec
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.
> > > > i
> > > > nf
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.
> > > > i
> > > > nf
> > > > index feae7b3e06..cf5bfe4083 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.
> > > > i
> > > > nf
> > > > +++
> > > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLi
> > > > +++ b.inf
> > > > @@ -57,3 +57,6 @@
> > > >  [Pcd]
> > > >    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard    # CONSUMES
> > > >
> > > > +[FeaturePcd]
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > > CONSUMES
> > > > +
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> > > > Lib.inf
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> > > > Lib.inf
> > > > index 967cb61ba6..8ae4feae62 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> > > > Lib.inf
> > > > +++
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandle
> > > > +++ rLib.inf
> > > > @@ -49,3 +49,7 @@
> > > >    LocalApicLib
> > > >    PeCoffGetEntryPointLib
> > > >    VmgExitLib
> > > > +
> > > > +[FeaturePcd]
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > > CONSUMES
> > > > +
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLi
> > > > b.inf
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLi
> > > > b.inf
> > > > index ea5b10b5c8..c9f20da058 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLi
> > > > b.inf
> > > > +++
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLi
> > > > +++ b.inf
> > > > @@ -53,3 +53,6 @@
> > > >    DebugLib
> > > >    VmgExitLib
> > > >
> > > > +[FeaturePcd]
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > > CONSUMES
> > > > +
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> > > > rAsm.nasm
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> > > > rAsm.nasm
> > > > index 26cae56cc5..ebe0eec874 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> > > > rAsm.nasm
> > > > +++
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandl
> > > > +++ erAsm.nasm
> > > > @@ -13,6 +13,7 @@
> > > >  ; Notes:
> > > >  ;
> > > >
> > > > ;-------------------------------------------------------------------
> > > > --
> > > > ---------
> > > > +%include "Nasm.inc"
> > > >
> > > >  ;
> > > >  ; CommonExceptionHandler()
> > > > @@ -23,6 +24,7 @@
> > > >  extern ASM_PFX(mErrorCodeFlag)    ; Error code flags for exceptions
> > > >  extern ASM_PFX(mDoFarReturnFlag)  ; Do far return flag  extern
> > > > ASM_PFX(CommonExceptionHandler)
> > > > +extern ASM_PFX(FeaturePcdGet (PcdCpuSmmStackGuard))
> > > >
> > > >  SECTION .data
> > > >
> > > > @@ -371,8 +373,48 @@ DoReturn:
> > > >      push    qword [rax + 0x18]       ; save EFLAGS in new location
> > > >      mov     rax, [rax]        ; restore rax
> > > >      popfq                     ; restore EFLAGS
> > > > -    DB      0x48               ; prefix to composite "retq" with next "retf"
> > > > -    retf                      ; far return
> > > > +
> > > > +    ; The follow algorithm is used for clear shadow stack token busy bit.
> > > > +    ; The comment is based on the sample shadow stack.
> > > > +    ; The sample shadow stack layout :
> > > > +    ; Address | Context
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFD0  |   FREE                  | it is 0xFD8|0x02|(LMA & CS.L), after
> > > > SAVEPREVSSP.
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFD8  |  Prev SSP               |
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFE0  |   RIP                   |
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFE8  |   CS                    |
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFF0  |  0xFF0 | BUSY           | BUSY flag cleared after CLRSSBSY
> > > > +    ;         +-------------------------+
> > > > +    ;  0xFF8  | 0xFD8|0x02|(LMA & CS.L) |
> > > > +    ;         +-------------------------+
> > > > +    ; Instructions for Intel Control Flow Enforcement Technology
> > > > + (CET) are
> > > > supported since NASM version 2.15.01.
> > > > +    push     rax                ; SSP should be 0xFD8 at this point
> > > > +    cmp      byte [dword ASM_PFX(FeaturePcdGet
> > > > (PcdCpuSmmStackGuard))], 0
> > > > +    jz       CetDone
> > > > +    mov      rax, cr4
> > > > +    and      rax, 0x800000      ; check if CET is enabled
> > > > +    jz       CetDone
> > > > +    mov      rax, 0x04          ; advance past cs:lip:prevssp;supervisor shadow
> > > > stack token
> > > > +    INCSSP_RAX                  ; After this SSP should be 0xFF8
> > > > +    SAVEPREVSSP                 ; now the shadow stack restore token will be
> > > > created at 0xFD0
> > > > +    READSSP_RAX                 ; Read new SSP, SSP should be 0x1000
> > > > +    push     rax
> > > > +    sub      rax, 0x10
> > > > +    CLRSSBSY_RAX                ; Clear token at 0xFF0, SSP should be 0 after
> > this
> > > > +    sub      rax, 0x20
> > > > +    RSTORSSP_RAX                ; Restore to token at 0xFD0, new SSP will be
> > > 0xFD0
> > > > +    pop      rax
> > > > +    mov      rax, 0x01          ; Pop off the new save token created
> > > > +    INCSSP_RAX                  ; SSP should be 0xFD8 now
> > > > +CetDone:
> > > > +    pop      rax                ; restore rax
> > > > +
> > > > +    DB       0x48               ; prefix to composite "retq" with next "retf"
> > > > +    retf                        ; far return
> > > >  DoIret:
> > > >      iretq
> > > >
> > > > diff --git
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> > > > HandlerLib.inf
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> > > > HandlerLib.inf
> > > > index 743c2aa766..a15f125d5b 100644
> > > > ---
> > > >
> > >
> > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> > > > HandlerLib.inf
> > > > +++
> > > >
> > >
> > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> > > > +++ HandlerLib.inf
> > > > @@ -54,3 +54,7 @@
> > > >    LocalApicLib
> > > >    PeCoffGetEntryPointLib
> > > >    VmgExitLib
> > > > +
> > > > +[FeaturePcd]
> > > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > > CONSUMES
> > > > +
> > > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > > b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > > index 28f8e8e133..7ef3b1d488 100644
> > > > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
> > > > @@ -173,6 +173,7 @@ InitShadowStack (  {
> > > >    UINTN       SmmShadowStackSize;
> > > >    UINT64      *InterruptSspTable;
> > > > +  UINT32      InterruptSsp;
> > > >
> > > >    if ((PcdGet32 (PcdControlFlowEnforcementPropertyMask) != 0) &&
> > > > mCetSupported) {
> > > >      SmmShadowStackSize = EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES
> > > > (PcdGet32 (PcdCpuSmmShadowStackSize))); @@ -191,7 +192,19 @@
> > > > InitShadowStack (
> > > >          ASSERT (mSmmInterruptSspTables != 0);
> > > >          DEBUG ((DEBUG_INFO, "mSmmInterruptSspTables - 0x%x\n",
> > > > mSmmInterruptSspTables));
> > > >        }
> > > > -      mCetInterruptSsp = (UINT32)((UINTN)ShadowStack +
> > > > EFI_PAGES_TO_SIZE(1) - sizeof(UINT64));
> > > > +
> > > > +      //
> > > > +      // The highest address on the stack (0xFF8) is a
> > > > + save-previous-ssp token
> > > > pointing to a location that is 40 bytes away - 0xFD0.
> > > > +      // The supervisor shadow stack token is just above it at address
> > 0xFF0.
> > > > This is where the interrupt SSP table points.
> > > > +      // So when an interrupt of exception occurs, we can use
> > > > SAVESSP/RESTORESSP/CLEARSSBUSY for the supervisor shadow stack,
> > > > +      // due to the reason the RETF in SMM exception handler cannot
> > > > + clear
> > > > the BUSY flag with same CPL.
> > > > +      // (only IRET or RETF with different CPL can clear BUSY flag)
> > > > +      // Please refer to
> > > > + UefiCpuPkg/Library/CpuExceptionHandlerLib/X64 for
> > > > the full stack frame at runtime.
> > > > +      //
> > > > +      InterruptSsp = (UINT32)((UINTN)ShadowStack +
> > > > + EFI_PAGES_TO_SIZE(1)
> > > > - sizeof(UINT64));
> > > > +      *(UINT32 *)(UINTN)InterruptSsp = (InterruptSsp - sizeof(UINT64)
> > > > + * 4) |
> > > > 0x2;
> > > > +      mCetInterruptSsp = InterruptSsp - sizeof(UINT64);
> > > > +
> > > >        mCetInterruptSspTable = (UINT32)(UINTN)(mSmmInterruptSspTables
> > > > +
> > > > sizeof(UINT64) * 8 * CpuIndex);
> > > >        InterruptSspTable = (UINT64 *)(UINTN)mCetInterruptSspTable;
> > > >        InterruptSspTable[1] = mCetInterruptSsp;
> > > > --
> > > > 2.16.2.windows.1
> > > >
> > > >
> > > >
> > > >
> > > >
> > >
> > >
> > >
> > >
> > >
> >
> >
> >
> > 
> >