One more question: 4) I saw from original author's note: The interrupt SSP table point should be 0xFF0.
I have not seen you update https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmmFuncsArch.c#L194 May I know that SSP table point is ? > -----Original Message----- > From: [email protected] <[email protected]> On Behalf Of Yao, Jiewen > Sent: Sunday, January 31, 2021 9:38 AM > To: Sheng, W <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Laszlo Ersek > <[email protected]>; Kumar, Rahul1 <[email protected]>; Yao, Jiewen > <[email protected]> > Subject: Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: > Clear CET shadow stack token busy bit > > Hi > I have some feedback. > > 1) Would you please confirm you have validated the > https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/Library/SmmCpuF > eaturesLib and > https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/PiSmmCpuDxeSm > m 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 <[email protected]> > > Sent: Friday, January 29, 2021 4:00 PM > > To: [email protected] > > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Laszlo > Ersek > > <[email protected]>; Kumar, Rahul1 <[email protected]>; Yao, > Jiewen > > <[email protected]> > > 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 <[email protected]> > > Cc: Eric Dong <[email protected]> > > Cc: Ray Ni <[email protected]> > > Cc: Laszlo Ersek <[email protected]> > > Cc: Rahul Kumar <[email protected]> > > Cc: Jiewen Yao <[email protected]> > > --- > > .../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 (#70924): https://edk2.groups.io/g/devel/message/70924 Mute This Topic: https://groups.io/mt/80205210/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
