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 748B02112A28C for ; Wed, 12 Sep 2018 03:41:44 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 96B644023820; Wed, 12 Sep 2018 10:41:43 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-31.rdu2.redhat.com [10.10.120.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3D1BCFA989; Wed, 12 Sep 2018 10:41:42 +0000 (UTC) To: "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: "Ni, Ruiyu" , "Justen, Jordan L" , "Yao, Jiewen" , "Zeng, Star" , Ard Biesheuvel References: <20180911051636.4888-1-jian.j.wang@intel.com> <20180911051636.4888-3-jian.j.wang@intel.com> <522bfb71-444c-2caa-a9ff-d5faf97c5a16@redhat.com> From: Laszlo Ersek Message-ID: <035c6517-dba6-ad27-5a25-b5ebf1653a1c@redhat.com> Date: Wed, 12 Sep 2018 12:41:41 +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: X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 12 Sep 2018 10:41:43 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Wed, 12 Sep 2018 10:41:43 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.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: Wed, 12 Sep 2018 10:41:44 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/12/18 04:11, Wang, Jian J wrote: > Hi Laszlo and Ard, > > Retiring the PcdSetNxForStack is not the intention of this patch series. It's just > a side effect of solving problem stated in BZ#1116. Sorry again for the misleading > title. I'm not insisting that we have to remove this PCD anyway (My personal > opinion. Others might have different ones). > > I think I understand the importance of PcdSetNxForStack to arm/aarch64 now. And I > agree that it'd be better to enable NX for stack independent of enabling NX for > EfiBootServcesData memory. But since the stack is type of EfiBootServicesData, > I think we should avoid any confusion in enabling/disabling NX for them. That's what > BZ#1116 tries to complain about. But I'm still not clear any concrete suggestion on > how to solve the BZ#1116 from all comment so far, if my patch series cannot satisfy > all kinds of platforms. Simply keep PcdSetNxForStack doesn't help, I think. (It doesn't > imply we must remove it.) > > As I commented in the BZ#1116, maybe we can just simply assert if there's one > trying to disable NX for stack but enable NX for EfiBootServicesData at the same time, > because it doesn't make sense. Yes, that's what I thought as well. Catch the one case with an assert and/or CpuDeadLoop() that is an invalid configuration. > I think all other setting combinations won't have > such confusion and don't need to take care specifically. > > And for x86 CPU, we'll always enable CPU NX feature if any NX related PCDs have bits > set. Current DxeIpl will just enable NX for PcdSetNxForStack only. My understanding has been that it's not about enabling/disabling a CPU feature, but about marking specific pages as non-executable. Under that interpretation, it should be possible to mark pages used for stack purposes as non-executable, and leave everything else executable, even on x86. Laszlo > > Regards, > Jian > > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Tuesday, September 11, 2018 11:53 PM > To: Wang, Jian J ; edk2-devel@lists.01.org > Cc: Ni, Ruiyu ; Justen, Jordan L ; Yao, Jiewen ; Zeng, Star ; Ard Biesheuvel > Subject: Re: [edk2] [PATCH 2/5] OvmfPkg/PlatformPei: expire the use of PcdSetNxForStack > > 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 >