HI JieWen, Thank you for the comment. I have updated the patch to v3 Hi All, The different between patch v2 and patch v3 is add more comment. Could you help to continue the review . Thank you. BR Sheng Wei
> -----Original Message----- > From: Yao, Jiewen <jiewen....@intel.com> > Sent: 2021年2月5日 17:59 > To: Sheng, W <w.sh...@intel.com>; devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Laszlo > Ersek <ler...@redhat.com>; Kumar, Rahul1 <rahul1.ku...@intel.com>; > Feng, Roger <roger.f...@intel.com> > Subject: RE: [PATCH v2 1/1] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET > shadow stack token busy bit > > 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 �C 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.sh...@intel.com> > > Sent: Friday, February 5, 2021 5:28 PM > > To: devel@edk2.groups.io > > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; > > Laszlo Ersek <ler...@redhat.com>; Kumar, Rahul1 > > <rahul1.ku...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; Feng, > > Roger <roger.f...@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.sh...@intel.com> > > Cc: Eric Dong <eric.d...@intel.com> > > Cc: Ray Ni <ray...@intel.com> > > Cc: Laszlo Ersek <ler...@redhat.com> > > Cc: Rahul Kumar <rahul1.ku...@intel.com> > > Cc: Jiewen Yao <jiewen....@intel.com> > > Cc: Roger Feng <roger.f...@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/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..05a802a633 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,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/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 > > + > > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71400): https://edk2.groups.io/g/devel/message/71400 Mute This Topic: https://groups.io/mt/80402161/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
0000-cover-letter.patch
Description: 0000-cover-letter.patch
0001-UefiCpuPkg-CpuExceptionHandlerLib-Clear-CET-shadow-s.patch
Description: 0001-UefiCpuPkg-CpuExceptionHandlerLib-Clear-CET-shadow-s.patch
