From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.65; helo=mga03.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) (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 A755F211518E1 for ; Fri, 21 Sep 2018 01:42:42 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 21 Sep 2018 01:42:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,284,1534834800"; d="scan'208";a="92491203" Received: from shzintpr04.sh.intel.com (HELO [10.7.209.18]) ([10.239.4.101]) by orsmga001.jf.intel.com with ESMTP; 21 Sep 2018 01:42:39 -0700 To: Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Laszlo Ersek , Jiewen Yao , star.zeng@intel.com References: <20180920060247.7764-1-jian.j.wang@intel.com> <20180920060247.7764-3-jian.j.wang@intel.com> From: "Zeng, Star" Message-ID: <4485f9fb-9320-9dbf-a344-a8b18825877b@intel.com> Date: Fri, 21 Sep 2018 16:42:09 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: Subject: Re: [PATCH v2 2/2] MdeModulePkg/DxeIpl: support more NX related PCDs 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, 21 Sep 2018 08:42:42 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Another minor suggestion is to move IsExecuteDisableBitAvailable() to VirtualMemory.c, then there will be no need to declare it in VirtualMemeory.h. Thanks, Star On 2018/9/21 14:00, Zeng, Star wrote: > Jian and Laszlo, > > There is also a superficial comment at below. > > On 2018/9/20 14:02, Jian J Wang wrote: >>> v2 changes: >>>     a. remove macros no longer needed >>>     b. remove DEBUG and ASSERT in ToEnableExecuteDisableFeature() >>>     c. change ToEnableExecuteDisableFeature to EnableNonExec >> >> BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 >> >> Currently IA32_EFER.NXE is only set against PcdSetNxForStack. This >> confuses developers because following two other PCDs also need NXE >> to be set, but actually not. >> >>      PcdDxeNxMemoryProtectionPolicy >>      PcdImageProtectionPolicy >> >> This patch solves this issue by adding logic to enable IA32_EFER.NXE >> if any of those PCDs have anything enabled. >> >> Cc: Star Zeng >> Cc: Laszlo Ersek >> Cc: Ard Biesheuvel >> Cc: Ruiyu Ni >> Cc: Jiewen Yao >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Jian J Wang >> --- >>   MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf          |  2 ++ >>   MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c  |  4 ++-- >>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c | 30 >> +++++++++++++++++++++++- >>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 24 >> +++++++++++++++++++ >>   4 files changed, 57 insertions(+), 3 deletions(-) >> >> diff --git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf >> b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf >> index fd82657404..068e700074 100644 >> --- a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf >> +++ b/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf >> @@ -117,6 +117,8 @@ >>   [Pcd.IA32,Pcd.X64,Pcd.ARM,Pcd.AARCH64] >>     gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack               ## >> SOMETIMES_CONSUMES >> +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## >> SOMETIMES_CONSUMES >> +  gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy       ## >> SOMETIMES_CONSUMES >>   [Depex] >>     gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid >> diff --git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c >> b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c >> index d28baa3615..ccd30f964b 100644 >> --- a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c >> +++ b/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c >> @@ -245,7 +245,7 @@ ToBuildPageTable ( >>       return TRUE; >>     } >> -  if (PcdGetBool (PcdSetNxForStack) && IsExecuteDisableBitAvailable >> ()) { >> +  if (EnableNonExec ()) { >>       return TRUE; >>     } >> @@ -436,7 +436,7 @@ HandOffToDxeCore ( >>       BuildPageTablesIa32Pae = ToBuildPageTable (); >>       if (BuildPageTablesIa32Pae) { >>         PageTables = Create4GPageTablesIa32Pae (BaseOfStack, STACK_SIZE); >> -      if (IsExecuteDisableBitAvailable ()) { >> +      if (EnableNonExec ()) { >>           EnableExecuteDisableBit(); >>         } >>       } >> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >> index 496e219913..73b0f67c6b 100644 >> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >> @@ -106,6 +106,31 @@ IsNullDetectionEnabled ( >>     return ((PcdGet8 (PcdNullPointerDetectionPropertyMask) & BIT0) != 0); >>   } >> +/** >> +  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not. >> + >> +  @retval TRUE    IA32_EFER.NXE should be enabled. >> +  @retval FALSE   IA32_EFER.NXE should not be enabled. >> + >> +**/ >> +BOOLEAN >> +EnableNonExec ( >> +  VOID >> +  ) >> +{ >> +  if (!IsExecuteDisableBitAvailable ()) { >> +    return FALSE; >> +  } >> + >> +  // >> +  // XD flag (BIT63) in page table entry is only valid if >> IA32_EFER.NXE is set. >> +  // Features controlled by Following PCDs need this feature to be >> enabled. >> +  // >> +  return (PcdGetBool (PcdSetNxForStack) || >> +          PcdGet64 (PcdDxeNxMemoryProtectionPolicy) != 0 || >> +          PcdGet32 (PcdImageProtectionPolicy) != 0); >> +} > > I am a little confused by this function name compared with > EnableExecuteDisableBit(). This function is not really to enable NX, but > just to check whether enable NX is needed or not. How about using name > IsEnableNonExecNeeded or IsEnableNxNeeded or IsDisableExecuteNeeded? > > > Sorry I did not raise this comment in V1 patch thread. > If you agree with the name changing, Reviewed-by: Star Zeng > . > > > Thanks, > Star >> + >>   /** >>     Enable Execute Disable Bit. >> @@ -755,7 +780,10 @@ CreateIdentityMappingPageTables ( >>     // >>     EnablePageTableProtection ((UINTN)PageMap, TRUE); >> -  if (PcdGetBool (PcdSetNxForStack)) { >> +  // >> +  // Set IA32_EFER.NXE if necessary. >> +  // >> +  if (EnableNonExec ()) { >>       EnableExecuteDisableBit (); >>     } >> diff --git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >> b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >> index 85457ff937..09085312aa 100644 >> --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >> +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >> @@ -179,6 +179,30 @@ typedef struct { >>     UINTN           FreePages; >>   } PAGE_TABLE_POOL; >> +/** >> +  Check if Execute Disable Bit (IA32_EFER.NXE) should be enabled or not. >> + >> +  @retval TRUE    IA32_EFER.NXE should be enabled. >> +  @retval FALSE   IA32_EFER.NXE should not be enabled. >> + >> +**/ >> +BOOLEAN >> +EnableNonExec ( >> +  VOID >> +  ); >> + >> +/** >> +  The function will check if Execute Disable Bit is available. >> + >> +  @retval TRUE      Execute Disable Bit is available. >> +  @retval FALSE     Execute Disable Bit is not available. >> + >> +**/ >> +BOOLEAN >> +IsExecuteDisableBitAvailable ( >> +  VOID >> +  ); >> + >>   /** >>     Enable Execute Disable Bit. >>