Re: [PATCH v2 1/1] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit

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

Would you please add comment on why we need reserve and program the 8 bytes here?
Something like:

//
// 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.
//

-      mCetInterruptSsp = (UINT32)((UINTN)ShadowStack + EFI_PAGES_TO_SIZE(1) - sizeof(UINT64));
+      InterruptSsp = (UINT32)((UINTN)ShadowStack + EFI_PAGES_TO_SIZE(1) - sizeof(UINT64));
+      *(UINT32 *)(UINTN)InterruptSsp = (InterruptSsp - sizeof(UINT64) * 4) | 0x2;
+      mCetInterruptSsp = InterruptSsp - sizeof(UINT64);

> -----Original Message-----
> From: Sheng, W <w.sheng@intel.com>
> Sent: Friday, February 5, 2021 5:28 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 v2 1/1] 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             | 48
> ++++++++++++++++++++--
>  .../Xcode5SecPeiCpuExceptionHandlerLib.inf         |  4 ++
>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c       |  5 ++-
>  7 files changed, 66 insertions(+), 4 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/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/PeiCpuExceptionHandlerLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> index feae7b3e06..cf5bfe4083 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib.inf
> @@ -57,3 +57,6 @@
>  [Pcd]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdCpuStackGuard    # CONSUMES
> 
> +[FeaturePcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> CONSUMES
> +
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i
> nf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i
> nf
> index 967cb61ba6..8ae4feae62 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i
> nf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandlerLib.i
> nf
> @@ -49,3 +49,7 @@
>    LocalApicLib
>    PeCoffGetEntryPointLib
>    VmgExitLib
> +
> +[FeaturePcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> CONSUMES
> +
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> index 4cdb11c04e..5c3d1f7cfd 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SmmCpuExceptionHandlerLib.inf
> @@ -53,3 +53,6 @@
>    DebugLib
>    VmgExitLib
> 
> +[FeaturePcd]
> +  gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmStackGuard                    ##
> CONSUMES
> +
> diff --git
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> index 26cae56cc5..05a802a633 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> m.nasm
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandlerAs
> 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,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
> +    DB 0xF3, 0x0F, 0x01, 0xEA   ; 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
> +    DB 0xF3, 0x0F, 0xAE, 0x30   ; CLRSSBSY RAX ; Clear token at 0xFF0 ; SSP
> should be 0 after this
> +    sub      rax, 0x20
> +    DB 0xF3, 0x0F, 0x01, 0x28   ; 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/Xcode5SecPeiCpuExceptionHan
> dlerLib.inf
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHan
> dlerLib.inf
> index 743c2aa766..a15f125d5b 100644
> ---
> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHan
> dlerLib.inf
> +++
> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHan
> dlerLib.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..1aa1102f56 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,9 @@ InitShadowStack (
>          ASSERT (mSmmInterruptSspTables != 0);
>          DEBUG ((DEBUG_INFO, "mSmmInterruptSspTables - 0x%x\n",
> mSmmInterruptSspTables));
>        }
> -      mCetInterruptSsp = (UINT32)((UINTN)ShadowStack + EFI_PAGES_TO_SIZE(1)
> - sizeof(UINT64));
> +      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