Hi Jiewen, Eric, Ray, Rahul, Ersek, I have updated the patch v2. And all comments are fixed. Since open CI is using NASM 2.14.02, it has not supported CET instructions yet. I would like to use DB xx xx xx xx to replace the assembly instruction before NASM 2.15.01 is used by open CI. Could you continue the code review ? Thank you. BR Sheng Wei
> -----Original Message----- > From: Sheng, W > Sent: 2021年2月3日 10:43 > To: Yao, Jiewen <[email protected]>; [email protected] > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; Laszlo > Ersek <[email protected]>; Kumar, Rahul1 <[email protected]> > Subject: RE: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: > Clear CET shadow stack token busy bit > > Hi Jiewen, > Thank you for the comments. > 1) > I have tried CET is working if set > gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmRestrictedMemoryAccess = TRUE > > 2) > 3) > I will add detail comments and add shadow stack layout in the source code > file. > > 4) > Sorry for miss the code in file > https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/PiSmmCpuDx > eSmm/X64/SmmFuncsArch.c > I will update the code in next patch set. > > I will raise patch V2 after we get the consensus of using BD xxx or update > NASM version for CET instruction support. > BR > Sheng Wei > > > > -----Original Message----- > > From: Yao, Jiewen <[email protected]> > > Sent: 2021年1月31日 9:47 > > To: [email protected]; Yao, Jiewen <[email protected]>; Sheng, > W > > <[email protected]> > > Cc: Dong, Eric <[email protected]>; Ni, Ray <[email protected]>; > > Laszlo Ersek <[email protected]>; Kumar, Rahul1 > > <[email protected]> > > Subject: RE: [edk2-devel] [PATCH 2/2] > UefiCpuPkg/CpuExceptionHandlerLib: > > Clear CET shadow stack token busy bit > > > > 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/PiSmmCpuDx > > eSmm/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/SmmC > > p > > > uF > > > eaturesLib and > > > > > > https://github.com/tianocore/edk2/tree/master/UefiCpuPkg/PiSmmCpuDx > > eSm > > > 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/PeiCpuExceptionHandlerLi > > > > b.inf > > > > > > b/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLi > > > > b.inf > > > > index feae7b3e06..cf5bfe4083 100644 > > > > --- > > > > > > a/UefiCpuPkg/Library/CpuExceptionHandlerLib/PeiCpuExceptionHandlerLi > > > > b.inf > > > > +++ > > > > > > 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 > > 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..13fd147f11 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,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/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 > > > > + > > > > -- > > > > 2.16.2.windows.1 > > > > > > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#71352): https://edk2.groups.io/g/devel/message/71352 Mute This Topic: https://groups.io/mt/80205210/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
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
