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.kin...@intel.com> > Cc: Liming Gao <liming....@intel.com> > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Yao Jiewen <jiewen....@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 <ler...@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
