[edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit
Yao, Jiewen
jiewen.yao at intel.com
Sun Jan 31 01:38:24 UTC 2021
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 at intel.com>
> Sent: Friday, January 29, 2021 4:00 PM
> To: devel at edk2.groups.io
> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Laszlo Ersek
> <lersek at redhat.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao, Jiewen
> <jiewen.yao at 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 at intel.com>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Rahul Kumar <rahul1.kumar at intel.com>
> Cc: Jiewen Yao <jiewen.yao at 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70923): https://edk2.groups.io/g/devel/message/70923
Mute This Topic: https://groups.io/mt/80205210/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list