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

["Sheng Wei" <w.sheng@intel.com>]

Hi Jiewen,
Thank you for the comments.
1) 
I have tried CET is working if set gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess = TRUE

2)
3)
I will add detail comments and add shadow stack layout in the source code file.

4) 
Sorry for miss the code in file https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c
I will update the code in next patch set.

I will raise patch V2 after we get the consensus of using BD xxx or update NASM version for CET instruction support.
BR
Sheng Wei


> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao@intel.com>
> Sent: 2021年1月31日 9:47
> To: devel@edk2.groups.io; Yao, Jiewen <jiewen.yao@intel.com>; 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>
> Subject: RE: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib:
> Clear CET shadow stack token busy bit
> 
> One more question:
> 
> 4) I saw from original author's note: The interrupt SSP table point should be
> 0xFF0.
> 
> I have not seen you update
> https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDx
> eSmm/X64/SmmFuncsArch.c#L194
> 
> May I know that SSP table point is ?
> 
> 
> 
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao,
> > Jiewen
> > Sent: Sunday, January 31, 2021 9:38 AM
> > 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>; Yao, Jiewen <jiewen.yao@intel.com>
> > Subject: Re: [edk2-devel] [PATCH 2/2]
> UefiCpuPkg/CpuExceptionHandlerLib:
> > Clear CET shadow stack token busy bit
> >
> > Hi
> > I have some feedback.
> >
> > 1) Would you please confirm you have validated the
> >
> https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/Library/SmmC
> p
> > uF
> > eaturesLib and
> >
> https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/PiSmmCpuDx
> eSm
> > m with dynamic paging turn on
> >
> (gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess|FALSE
> ),
> > and with multiple page fault triggered in the code?
> >
> > 2) Would you please add comment for the assembly instruction?
> >
> > I saw good comment from the original author. Not sure why you removed
> them?
> >
> > 	   push %rax               ; SSP should be 0xFD8 at this point
> > 	   mov $0x04, %rax         ; advance past cs:lip:prevssp;supervisor
> shadow
> > stack token
> > 	   INCSSP %rax            ; After this SSP should be 0xFF8
> > 	   SAVEPREVSSP             ; now s shadow stack restore token will be
> > created at 0xFD0
> > 	   RDSSP %rax              ; Read new SSP - should be 0x1000
> > 	   CLRSSBSY (%rax - $0x10) ; Clear token at 0xFF0; SSP should be 0
> > after this
> > 	   RESTORESSP (%rax - $0x30) ; Restore to token at 0xFD0 - new SSP
> > will be 0xFD0
> > 	   Mov $0x01, %rax         ; Pop off the new save token created
> > 	   INCSSP %rax            ; SSP should be 0xFD8 now
> > 	   pop %rax                ; restore rax
> > 	   Retf                    ; Return
> >
> > 3) Please draw the stack layout in the file. It will help other people
> > maintain the code later.
> >
> > For example:
> >
> >             +------------------------------------+
> > 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) |
> >             +------------------------------------+
> >
> > Thank you
> > Yao Jiewen
> >
> >
> > > -----Original Message-----
> > > From: Sheng, W <w.sheng@intel.com>
> > > Sent: Friday, January 29, 2021 4:00 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>
> > > Subject: [PATCH 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.
> > > The busy bit should be cleared when return back form SMM exception
> > > to SMM handler. Otherwise, keeping busy bit in set state 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>
> > > ---
> > >  .../DxeCpuExceptionHandlerLib.inf                  |  3 +++
> > >  .../PeiCpuExceptionHandlerLib.inf                  |  3 +++
> > >  .../SecPeiCpuExceptionHandlerLib.inf               |  4 ++++
> > >  .../SmmCpuExceptionHandlerLib.inf                  |  3 +++
> > >  .../X64/Xcode5ExceptionHandlerAsm.nasm             | 28
> > > +++++++++++++++++++++-
> > >  .../Xcode5SecPeiCpuExceptionHandlerLib.inf         |  4 ++++
> > >  6 files changed, 44 insertions(+), 1 deletion(-)
> > >
> > > 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/DxeCpuExceptionHandlerLib.
> > 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/PeiCpuExceptionHandlerLi
> > > b.inf
> > >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLi
> > > b.inf
> > > index feae7b3e06..cf5bfe4083 100644
> > > ---
> > >
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLi
> > > b.inf
> > > +++
> > >
> 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
> L
> > ib.i
> > > nf
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> L
> > ib.i
> > > nf
> > > index 967cb61ba6..8ae4feae62 100644
> > > ---
> > >
> >
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> L
> > ib.i
> > > nf
> > > +++
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler
> L
> > ib.i
> > > nf
> > > @@ -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 4cdb11c04e..5c3d1f7cfd 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
> r
> > As
> > > m.nasm
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> r
> > As
> > > m.nasm
> > > index 26cae56cc5..13fd147f11 100644
> > > ---
> > >
> >
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> r
> > As
> > > m.nasm
> > > +++
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle
> r
> > As
> > > m.nasm
> > > @@ -1,5 +1,5 @@
> > >
> > > ;-------------------------------------------------------------------
> > > ----------- ; -; Copyright (c) 2012 - 2018, Intel Corporation. All
> > > rights reserved.<BR>
> > > +; Copyright (c) 2012 - 2021, Intel Corporation. All rights
> > > +reserved.<BR>
> > >  ; SPDX-License-Identifier: BSD-2-Clause-Patent  ;  ; Module Name:
> > > @@ -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,6 +373,30 @@ DoReturn:
> > >      push    qword [rax + 0x18]       ; save EFLAGS in new location
> > >      mov     rax, [rax]        ; restore rax
> > >      popfq                     ; restore EFLAGS
> > > +
> > > +    push    rax
> > > +    cmp     byte [dword ASM_PFX(FeaturePcdGet
> (PcdCpuSmmStackGuard))], 0
> > > +    jz      CetDone
> > > +    mov     rax, cr4
> > > +    and     rax, 0x800000     ; check if CET is enabled
> > > +    jz      CetDone
> > > +    push    rbx
> > > +    mov     rax, 0x04
> > > +    INCSSP_RAX
> > > +    SAVEPREVSSP
> > > +    READSSP_RAX
> > > +    mov     rbx, rax
> > > +    sub     rax, 0x10
> > > +    CLRSSBSY_RAX
> > > +    mov     rax, rbx
> > > +    sub     rax, 0x30
> > > +    RSTORSSP_RAX
> > > +    mov     rax, 0x01
> > > +    INCSSP_RAX
> > > +    pop     rbx
> > > +CetDone:
> > > +    pop     rax
> > > +
> > >      DB      0x48               ; prefix to composite "retq" with next "retf"
> > >      retf                      ; far return
> > >  DoIret:
> > > diff --git
> > >
> >
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> Ha
> > n
> > > dlerLib.inf
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> Ha
> > n
> > > dlerLib.inf
> > > index 743c2aa766..a15f125d5b 100644
> > > ---
> > >
> >
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> Ha
> > n
> > > dlerLib.inf
> > > +++
> > >
> >
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException
> Ha
> > n
> > > dlerLib.inf
> > > @@ -54,3 +54,7 @@
> > >    LocalApicLib
> > >    PeCoffGetEntryPointLib
> > >    VmgExitLib
> > > +
> > > +[FeaturePcd]
> > > +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> > > CONSUMES
> > > +
> > > --
> > > 2.16.2.windows.1
> >
> >
> >
> > 
> >