Thank you. I have reviewed that. We still need UefiCpuPkg and MdePkg maintainer's review before merge.
Thank you Yao Jiewen > -----Original Message----- > From: Sheng, W <w.sh...@intel.com> > Sent: Thursday, February 25, 2021 1:58 PM > To: devel@edk2.groups.io; Yao, Jiewen <jiewen....@intel.com>; Sheng, W > <w.sh...@intel.com>; Ni, Ray <ray...@intel.com> > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > Kumar, Rahul1 <rahul1.ku...@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, > I have refined the comment in the code. > It is working with PcdCpuSmmRestrictedMemoryAccess enabled. > Add the sample code in file > https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDxeSm > m/X64/SmmFuncsArch.c > [PATCH v5 1/2] is the patch of add CET instruction DX define in nasm.inc file. > https://edk2.groups.io/g/devel/message/72182 > > Do have any comment for the patch of fix CET shadow stack token busy bit > issue ? > Could you give review-by for 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月23日 15:52 > > To: devel@edk2.groups.io; Sheng, W <w.sh...@intel.com>; Ni, Ray > > <ray...@intel.com>; Yao, Jiewen <jiewen....@intel.com> > > Cc: Dong, Eric <eric.d...@intel.com>; Laszlo Ersek <ler...@redhat.com>; > > Kumar, Rahul1 <rahul1.ku...@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, 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 (#72218): https://edk2.groups.io/g/devel/message/72218 Mute This Topic: https://groups.io/mt/80896608/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
