Hi Jiewen, Ray, Could you help to review and give Review-by for this patch for fix CET shadow stack token busy bit issue ? As the common in v5 patch 1/2 by Limin, since it is a bug fix, it can still be merged in 202102 stable tag soft feature freeze phase. https://edk2.groups.io/g/devel/message/72013 Thank you. BR Sheng Wei
> -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sheng > Wei > Sent: 2021年2月22日 10:15 > To: devel@edk2.groups.io; Sheng, W <w.sh...@intel.com> > 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: Re: [edk2-devel] [PATCH v5 2/2] > UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit > > Hi Jiewen, > Thank you for review the patch. > Could you give review-by on this patch? > Thank you. > BR > Sheng Wei > > > > -----Original Message----- > > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Sheng > > Wei > > Sent: 2021年2月20日 11:15 > > 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: [edk2-devel] [PATCH v5 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. > > 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 | 46 > > +++++++++++++++++++++- > > .../Xcode5SecPeiCpuExceptionHandlerLib.inf | 4 ++ > > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c | 15 ++++++- > > 7 files changed, 75 insertions(+), 3 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/DxeCpuExceptionHandlerLi > > +++ b.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. > > i > > nf > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib. > > i > > nf > > index feae7b3e06..cf5bfe4083 100644 > > --- > > > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLib. > > i > > nf > > +++ > > 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 > > Lib.inf > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler > > Lib.inf > > index 967cb61ba6..8ae4feae62 100644 > > --- > > > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandler > > Lib.inf > > +++ > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/SecPeiCpuExceptionHandle > > +++ rLib.inf > > @@ -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 ea5b10b5c8..c9f20da058 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 > > rAsm.nasm > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle > > rAsm.nasm > > index 26cae56cc5..ebe0eec874 100644 > > --- > > > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandle > > rAsm.nasm > > +++ > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/Xcode5ExceptionHandl > > +++ erAsm.nasm > > @@ -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 > > + 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 > > + CLRSSBSY_RAX ; Clear token at 0xFF0, SSP should be 0 > > after this > > + sub rax, 0x20 > > + 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 > > HandlerLib.inf > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException > > HandlerLib.inf > > index 743c2aa766..a15f125d5b 100644 > > --- > > > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException > > HandlerLib.inf > > +++ > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuException > > +++ HandlerLib.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..7ef3b1d488 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,19 @@ > > InitShadowStack ( > > ASSERT (mSmmInterruptSspTables != 0); > > DEBUG ((DEBUG_INFO, "mSmmInterruptSspTables - 0x%x\n", > > mSmmInterruptSspTables)); > > } > > - mCetInterruptSsp = (UINT32)((UINTN)ShadowStack + > > EFI_PAGES_TO_SIZE(1) - sizeof(UINT64)); > > + > > + // > > + // 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. > > + // > > + 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 (#72040): https://edk2.groups.io/g/devel/message/72040 Mute This Topic: https://groups.io/mt/80846606/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
