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 2E5AD2114B12F for ; Fri, 21 Sep 2018 03:14:13 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9C8E2356C5; Fri, 21 Sep 2018 10:14:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-8.rdu2.redhat.com [10.10.120.8]) by smtp.corp.redhat.com (Postfix) with ESMTP id 3EBEC4F83; Fri, 21 Sep 2018 10:14:11 +0000 (UTC) To: "Zeng, Star" , Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jiewen Yao References: <20180920060247.7764-1-jian.j.wang@intel.com> <20180920060247.7764-3-jian.j.wang@intel.com> <4485f9fb-9320-9dbf-a344-a8b18825877b@intel.com> From: Laszlo Ersek Message-ID: <828d0685-5e4e-aab9-3195-c995679791c7@redhat.com> Date: Fri, 21 Sep 2018 12:14:10 +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: <4485f9fb-9320-9dbf-a344-a8b18825877b@intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.23 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Fri, 21 Sep 2018 10:14:12 +0000 (UTC) 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 10:14:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/21/18 10:42, Zeng, Star wrote: > Another minor suggestion is to move IsExecuteDisableBitAvailable() to > VirtualMemory.c, then there will be no need to declare it in > VirtualMemeory.h. I'm fine with both ideas (name change as you see fit, and code movement). Thanks Laszlo > > > 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. >>> >