From: "Yao, Jiewen" <jiewen.yao@intel.com>
To: "devel@edk2.groups.io" <devel@edk2.groups.io>,
"Yao, Jiewen" <jiewen.yao@intel.com>,
"Sheng, W" <w.sheng@intel.com>
Cc: "Dong, Eric" <eric.dong@intel.com>, "Ni, Ray" <ray.ni@intel.com>,
"Laszlo Ersek" <lersek@redhat.com>,
"Kumar, Rahul1" <rahul1.kumar@intel.com>
Subject: Re: [edk2-devel] [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit
Date: Sun, 31 Jan 2021 01:46:33 +0000 [thread overview]
Message-ID: <BY5PR11MB416640FC61E07A47C799EF8D8CB79@BY5PR11MB4166.namprd11.prod.outlook.com> (raw)
In-Reply-To: <165F2D66EAF4D84C.16314@groups.io>
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: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Yao, Jiewen
> Sent: Sunday, January 31, 2021 9:38 AM
> To: Sheng, W <w.sheng@intel.com>; devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao, Jiewen
> <jiewen.yao@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/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 <w.sheng@intel.com>
> > Sent: Friday, January 29, 2021 4:00 PM
> > To: devel@edk2.groups.io
> > Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo
> Ersek
> > <lersek@redhat.com>; Kumar, Rahul1 <rahul1.kumar@intel.com>; Yao,
> Jiewen
> > <jiewen.yao@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@intel.com>
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Cc: Rahul Kumar <rahul1.kumar@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@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/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
>
>
>
>
>
next prev parent reply other threads:[~2021-01-31 1:46 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-29 7:59 [PATCH 0/2] Fix #DF issue when enable CET shadow stack feature Sheng Wei
2021-01-29 7:59 ` [PATCH 1/2] MdePkg/Include: Add CET instructions to Nasm.inc Sheng Wei
2021-01-29 7:59 ` [PATCH 2/2] UefiCpuPkg/CpuExceptionHandlerLib: Clear CET shadow stack token busy bit Sheng Wei
2021-01-31 1:38 ` Yao, Jiewen
[not found] ` <165F2D66EAF4D84C.16314@groups.io>
2021-01-31 1:46 ` Yao, Jiewen [this message]
2021-02-03 2:43 ` [edk2-devel] " Sheng Wei
2021-02-05 9:35 ` Sheng Wei
2021-02-05 15:13 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY5PR11MB416640FC61E07A47C799EF8D8CB79@BY5PR11MB4166.namprd11.prod.outlook.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox