From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id C526521B02822 for ; Fri, 22 Feb 2019 04:00:39 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id B61F7308FE9E; Fri, 22 Feb 2019 12:00:38 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-51.rdu2.redhat.com [10.10.120.51]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6EE5260BE6; Fri, 22 Feb 2019 12:00:37 +0000 (UTC) To: Jiewen Yao , edk2-devel@lists.01.org Cc: Eric Dong , Liming Gao , Michael D Kinney References: <20190222041558.25312-1-jiewen.yao@intel.com> From: Laszlo Ersek Message-ID: <6b9678c5-9a5c-1868-3d47-5e1c99304214@redhat.com> Date: Fri, 22 Feb 2019 13:00:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190222041558.25312-1-jiewen.yao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.49]); Fri, 22 Feb 2019 12:00:38 +0000 (UTC) Subject: Re: [PATCH 0/3] Add SMM CET support X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 22 Feb 2019 12:00:40 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Liming Gao > Cc: Eric Dong > Cc: Ray Ni > Cc: Laszlo Ersek > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Yao Jiewen > > 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 (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 , 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 . 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