Re: [PATCH 0/3] Add SMM CET support

["Yao, Jiewen" <jiewen.yao@intel.com>]

The V3 patch is posted.
I add NASM.INC files.

Thank you
Yao Jiewen


> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> Yao, Jiewen
> Sent: Friday, February 22, 2019 8:11 PM
> To: Laszlo Ersek <lersek@redhat.com>; edk2-devel@lists.01.org
> Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric
> <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: Re: [edk2] [PATCH 0/3] Add SMM CET support
> 
> Thanks Laszlo.
> 
> 2) I have checked NASM instruction list at
> https://www.nasm.us/xdoc/2.14.02/html/nasmdocb.html
> SSP related instruction is not there.
> 
> I believe using DB maybe the only choice at this moment.
> 
> I will create include file.
> 
> 3) I will fix comment. Thanks to catch that.
> 
> 
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of
> > Laszlo Ersek
> > Sent: Friday, February 22, 2019 8:01 PM
> > To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> > Cc: Kinney, Michael D <michael.d.kinney@intel.com>; Dong, Eric
> > <eric.dong@intel.com>; Gao, Liming <liming.gao@intel.com>
> > Subject: Re: [edk2] [PATCH 0/3] Add SMM CET support
> >
> > On 02/22/19 05:15, Jiewen Yao wrote:
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1521
> > >
> > > This patch series implement add CET ShadowStack support for SMM.
> > >
> > > The CET document can be found at:
> > >
> >
> https://software.intel.com/sites/default/files/managed/4d/2a/control-flow
> > -enforcement-technology-preview.pdf
> > >
> > > Patch 1 adds SSP (ShadowStackPointer) to JUMP_BUFFER.
> > > Patch 2 adds Control Protection exception (CP#) dump info.
> > > Patch 3 adds CET ShadowStack support in SMM.
> > >
> > > For more detail please refer to each patch.
> > >
> > > I also post all update to https://github.com/jyao1/edk2/tree/CET
> > >
> > > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > > Cc: Liming Gao <liming.gao@intel.com>
> > > Cc: Eric Dong <eric.dong@intel.com>
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Cc: Laszlo Ersek <lersek@redhat.com>
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Yao Jiewen <jiewen.yao@intel.com>
> > >
> > > Jiewen Yao (3):
> > >   MdePkg/BaseLib: Add Shadow Stack Support for X86.
> > >   UefiCpuPkg/ExceptionLib: Add CET support.
> > >   UefiCpuPkg/PiSmmCpu: Add Shadow Stack Support for X86 SMM.
> >
> >
> > (1) For the series, in my usual environment:
> >
> > Regression-tested-by: Laszlo Ersek <lersek@redhat.com>
> >
> >
> > (2) I notice that the NASM code receives a bunch of DB encodings for
> > various instructions. I think that's a bad idea. It was pretty difficult
> > to eliminate DBs; please refer to
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=866>, and the commit
> > range aae02dccf5b0..d22c995a4814.
> >
> > As far as I can see, the DBs are added to encode three instructions,
> > namely READSSP, INCSSP, and SETSSBSY. Can you please confirm that the
> > only reason we use DBs for these instructions is that they are related
> > to the CET extension, and they are not yet supported by NASM? (Or at
> > least not by the NASM that that we require?)
> >
> > In other words, I'd like to be sure that the DBs are not used for
> > runtime instruction patching.
> >
> > Even that way, I think it would be better to use NASM macros for these
> > instructions. The code doesn't use many forms:
> >
> > * SETSSBSY:       DB 0xF3, 0x0F, 0x01, 0xE8
> > * READSSP EAX:    DB 0xF3, 0x0F, 0x1E, 0xC8
> > * INCSSP EAX:     DB 0xF3, 0x0F, 0xAE, 0xE8
> > * READSSP RAX:    DB 0xF3, 0x48, 0x0F, 0x1E, 0xC8
> > * INCSSP RAX:     DB 0xF3, 0x48, 0x0F, 0xAE, 0xE8
> >
> > (It seems that the EAX <-> RAX encodings, for READSSP and INCSSP, are
> > differentiated through the 0x48 REX.W prefix (64-bit operand size).)
> >
> > I think we should add the macros in a NASM include file under
> > "MdePkg/Include". Later, only those macros would have to be updated,
> > once NASM starts supporting these instructions directly.
> >
> > We've supported shared NASM include files since
> > <https://bugzilla.tianocore.org/show_bug.cgi?id=1085>. Therefore, both
> > UefiCpuPkg and MdePkg modules could consume the macros, from under
> > MdePkg/Include.
> >
> >
> > (3) In fact, looking at the DB encodings, I think some of the comments
> > are incorrect. Namely, in patch #3, in file
> > "UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/Cet.nasm", function DisableCet, we
> > have
> >
> > +    DB      0xF3, 0x0F, 0xAE, 0xE8       ; INCSSP RAX
> >
> > but that's INCSSP EAX, not RAX, in reality. (The code is correct, the
> > comment is wrong.) Using NASM macros would help us avoid such typos.
> >
> > Thanks
> > Laszlo
> >
> > _______________________________________________
> > edk2-devel mailing list
> > edk2-devel@lists.01.org
> > https://lists.01.org/mailman/listinfo/edk2-devel
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel