[edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit
Sheng Wei
w.sheng at intel.com
Wed Feb 3 02:43:02 UTC 2021
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/PiSmmCpuDxeSmm/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 <jiewen.yao at intel.com>
> Sent: 2021年1月31日 9:47
> To: devel at edk2.groups.io; Yao, Jiewen <jiewen.yao at intel.com>; Sheng, W
> <w.sheng at intel.com>
> Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Laszlo
> Ersek <lersek at redhat.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>
> 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: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Yao,
> > Jiewen
> > Sent: Sunday, January 31, 2021 9:38 AM
> > To: Sheng, W <w.sheng at intel.com>; devel at edk2.groups.io
> > Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> > Laszlo Ersek <lersek at redhat.com>; Kumar, Rahul1
> > <rahul1.kumar at intel.com>; Yao, Jiewen <jiewen.yao at intel.com>
> > 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 <w.sheng at intel.com>
> > > Sent: Friday, January 29, 2021 4:00 PM
> > > To: devel at edk2.groups.io
> > > Cc: Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>;
> > > Laszlo
> > Ersek
> > > <lersek at redhat.com>; Kumar, Rahul1 <rahul1.kumar at intel.com>; Yao,
> > Jiewen
> > > <jiewen.yao at intel.com>
> > > 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 <w.sheng at intel.com>
> > > Cc: Eric Dong <eric.dong at intel.com>
> > > Cc: Ray Ni <ray.ni at intel.com>
> > > Cc: Laszlo Ersek <lersek at redhat.com>
> > > Cc: Rahul Kumar <rahul1.kumar at intel.com>
> > > Cc: Jiewen Yao <jiewen.yao at intel.com>
> > > ---
> > > .../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 (#71086): https://edk2.groups.io/g/devel/message/71086
Mute This Topic: https://groups.io/mt/80205210/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list