From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 B124521B02822 for ; Tue, 11 Sep 2018 08:53:21 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id ED4BCE9006; Tue, 11 Sep 2018 15:53:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-36.rdu2.redhat.com [10.10.120.36]) by smtp.corp.redhat.com (Postfix) with ESMTP id A268310EE6C7; Tue, 11 Sep 2018 15:53:19 +0000 (UTC) To: Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jordan Justen , Jiewen Yao , Star Zeng , Ard Biesheuvel References: <20180911051636.4888-1-jian.j.wang@intel.com> <20180911051636.4888-3-jian.j.wang@intel.com> From: Laszlo Ersek Message-ID: <522bfb71-444c-2caa-a9ff-d5faf97c5a16@redhat.com> Date: Tue, 11 Sep 2018 17:53:18 +0200 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: <20180911051636.4888-3-jian.j.wang@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 11 Sep 2018 15:53:21 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Tue, 11 Sep 2018 15:53:21 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack 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: Tue, 11 Sep 2018 15:53:22 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/11/18 07:16, Jian J Wang wrote: > BZ#: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > > Since PcdSetNxForStack is expired, remove related config code. > PcdDxeNxMemoryProtectionPolicy cannot be used as dynamic PCD. > There's no need to add it in code to replace PcdSetNxForStack. > > Cc: Laszlo Ersek > Cc: Star Zeng > Cc: Jordan Justen > Cc: Ard Biesheuvel > Cc: Ruiyu Ni > Cc: Jiewen Yao > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Jian J Wang > --- > OvmfPkg/PlatformPei/Platform.c | 1 - > OvmfPkg/PlatformPei/PlatformPei.inf | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c > index 5a78668126..6627d236e0 100644 > --- a/OvmfPkg/PlatformPei/Platform.c > +++ b/OvmfPkg/PlatformPei/Platform.c > @@ -340,7 +340,6 @@ NoexecDxeInitialization ( > ) > { > UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdPropertiesTableEnable); > - UPDATE_BOOLEAN_PCD_FROM_FW_CFG (PcdSetNxForStack); > } > > VOID > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf > index 9c5ad9961c..ef5dcb7693 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -96,7 +96,6 @@ > gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration > gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode > gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable > - gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack > gEfiMdeModulePkgTokenSpaceGuid.PcdPropertiesTableEnable > gEfiMdeModulePkgTokenSpaceGuid.PcdAcpiS3Enable > gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask > I disagree with this change. I explained my reasons in , but for the sake of discussion, I'll state the same here: > Ard's got a point here. Just because one PCD implies another, the > reverse is not necessarily true. > > For example, in OVMF, we set PcdSetNxForStack to TRUE originally, but > then we had to make it conditional, due to old GRUB variants breaking > with a non-executable stack. (Please refer to commit d20b06a3afdf, > "OvmfPkg: disable no-exec DXE stack by default", 2015-09-15). > Therefore we now default to FALSE, and let the user set it to TRUE on > the QEMU command line. > > In addition, we intentionally inherit a zero > PcdDxeNxMemoryProtectionPolicy from "MdeModulePkg.dec". > > Both of the above configurations satisfy the requirement > > ((PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & > (1 << EfiBootServicesData)) == 0 || > PcdGetBool (PcdSetNxForStack)) > > i.e. the requirement that "NX for BS data" imply "NX for stack". > > If you remove the standalone knob for PcdSetNxForStack, then the > default behavior of OVMF will continue to work, however the command > line option, for setting PcdSetNxForStack *only*, will break. I'd like to add another comment just here (not mentioned in the BZ): To my understanding, the Heap Guard is a debug feature [1]. Over time, I've reviewed and regression-tested all the Heap Guard patches that have crossed my path with the understanding that "this is not enabled by default in OVMF". With those points in mind, I certainly don't intend to enable the Heap Guard as a FixedPCD -- which is the only way it can be enabled -- in the OVMF DSC files anytime soon. On the other hand, the user should set the stack NX preferably at all times -- as I wrote above, that was our original idea for the OVMF default as well, until we were forced to revert it for compatibility's sake, and to expose the knob to the end-user instead. With this patch series, the configuration that's currently deemed the best compromise for OVMF (Heap Guard off, stack NX) would be lost. [1] http://mid.mail-archive.com/D827630B58408649ACB04F44C510003624DDC8B3@SHSMSX103.ccr.corp.intel.com Thanks Laszlo