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

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

Hi
I have some feedback.

1) Would you please confirm you have validated the https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/Library/SmmCpuFeaturesLib and https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/PiSmmCpuDxeSmm 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/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..13fd147f11 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,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/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
> +
> --
> 2.16.2.windows.1