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

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

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