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 0BFD221136007 for ; Mon, 17 Sep 2018 03:13:42 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.25]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 545CEC04BE28; Mon, 17 Sep 2018 10:13:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-227.rdu2.redhat.com [10.10.120.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6EBB2201588F; Mon, 17 Sep 2018 10:13:39 +0000 (UTC) To: "Zeng, Star" , "Wang, Jian J" , "edk2-devel@lists.01.org" Cc: Ard Biesheuvel , "Ni, Ruiyu" , "Yao, Jiewen" References: <20180914051335.2644-1-jian.j.wang@intel.com> <9d5d297a-4d10-3ffd-3d02-1f369cfe5bda@redhat.com> <0C09AFA07DD0434D9E2A0C6AEB0483103BBBA7A4@shsmsx102.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <397ee56b-0c32-09a7-5542-ea6f81e6c211@redhat.com> Date: Mon, 17 Sep 2018 12:13:38 +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: <0C09AFA07DD0434D9E2A0C6AEB0483103BBBA7A4@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.25 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.31]); Mon, 17 Sep 2018 10:13:41 +0000 (UTC) Subject: Re: [PATCH] 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: Mon, 17 Sep 2018 10:13:43 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit On 09/17/18 07:57, Zeng, Star wrote: > How about we see the problem in another way? > > If my understanding is correct, current discussion and patches think FALSE/0 means disable/clear NX, but that is not the fact. > According to the code implementation, FALSE/0 seems mean *AS IS* to do thing (no code to disable/clear NX). > > PcdSetNxForStack > TRUE: Set NX for stack. > FALSE: No code to clear NX for stack. > > PcdDxeNxMemoryProtectionPolicy > BITX 1: Set NX for that memory type. > BITX 0: No code to clear NX for that memory type. > > PcdImageProtectionPolicy > BITX 1: Set NX for the image data section. > BITX 0: Not code to clear NX for the image data section. > > So, how about we think one PCD just works for itself and it does not impact other PCDs to protect? > That means TRUE/1 is to protect and FALSE/0 is *AS IS* to do nothing. > The description of these PCDs could be enhancement if we think it is a good way to see the problem. Sure, that too could work for me, but then the documentation in the DEC / UNI files has to be really clear. The initial worry for the current discussion was that some platform might - protect e.g. BootServicesData type memory, - not set PcdSetNxForStack, - expect the stack to remain executable. The actual results might surprise the platform owner. If the documentation dispelled any possible misconceptions, I think your idea could work too (and it would be a lot easier to code). Thanks Laszlo > > > Thanks, > Star > -----Original Message----- > From: Wang, Jian J > Sent: Monday, September 17, 2018 10:11 AM > To: Laszlo Ersek ; edk2-devel@lists.01.org > Cc: Zeng, Star ; Ard Biesheuvel ; Ni, Ruiyu ; Yao, Jiewen > Subject: RE: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs > > Laszlo, > > Thanks for the comments. > > Regards, > Jian > >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Friday, September 14, 2018 5:51 PM >> To: Wang, Jian J ; edk2-devel@lists.01.org >> Cc: Zeng, Star ; Ard Biesheuvel >> ; Ni, Ruiyu ; Yao, >> Jiewen >> Subject: Re: [PATCH] MdeModulePkg/DxeIpl: support more NX related PCDs >> >> I've got some comments on the code as well: >> >> On 09/14/18 07:13, Jian J Wang wrote: >>>  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. >>> >>>  Due to the fact that NX memory type of stack (enabled by  >>> PcdSetNxForStack) >>>  and image data section (enabled by PcdImageProtectionPolicy) are  >>> also >>>  part of PcdDxeNxMemoryProtectionPolicy, this patch also add more  >>> checks >>>  to warn (ASSERT) users any unreasonable setting combinations. For  >>> example, >>> >>>      PcdSetNxForStack == FALSE && >>>        (PcdDxeNxMemoryProtectionPolicy & (1 <>> != 0 >>> >>>      PcdImageProtectionPolicy == 0 && >>>        (PcdDxeNxMemoryProtectionPolicy & (1 <<  >>> EfiRuntimeServicesData)) != 0 >>> >>>      PcdImageProtectionPolicy == 0 && >>>        (PcdDxeNxMemoryProtectionPolicy & (1 <>> != 0 >>> >>>      PcdImageProtectionPolicy == 0 && >>>        (PcdDxeNxMemoryProtectionPolicy & (1 <>> >>>  In other words, PcdSetNxForStack and PcdImageProtectionPolicy have >>>  priority over PcdDxeNxMemoryProtectionPolicy. >>> >>>  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 | 55 +++++++++++++ >> ++++++++++- >>>   MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h | 33 +++++++++++++ >> + >>>   4 files changed, 91 insertions(+), 3 deletions(-) >>> >>>  diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf  >> b/MdeModulePkg/Core/DxeIp lPeim/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               ##  >>> SOMETIM >> ES_CONSUMES >>>  +  gEfiMdeModulePkgTokenSpaceGuid.PcdDxeNxMemoryProtectionPolicy ## >> SOMETIMES_CONSUMES >>>  +  gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy       ##  >>> SOME >> TIMES_CONSUMES >>> >>>   [Depex] >>>     gEfiPeiLoadFilePpiGuid AND gEfiPeiMasterBootModePpiGuid >>>  diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/Ia32/DxeLoadFunc.c b/MdeModulePkg/ >> Core/DxeIplPeim/Ia32/DxeLoadFunc.c >>>  index d28baa3615..9a97205ef8 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 (ToEnableExecuteDisableFeature ()) { >>>       return TRUE; >>>     } >>> >>>  @@ -436,7 +436,7 @@ HandOffToDxeCore ( >>>       BuildPageTablesIa32Pae = ToBuildPageTable (); >>>       if (BuildPageTablesIa32Pae) { >>>         PageTables = Create4GPageTablesIa32Pae (BaseOfStack,  >>> STACK_SIZE); >>>  -      if (IsExecuteDisableBitAvailable ()) { >>>  +      if (ToEnableExecuteDisableFeature ()) { >>>           EnableExecuteDisableBit(); >>>         } >>>       } >>>  diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c b/MdeModulePkg >> /Core/DxeIplPeim/X64/VirtualMemory.c >>>  index 496e219913..253fe84223 100644 >>>  --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >>>  +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.c >>>  @@ -106,6 +106,56 @@ 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 >>>  +ToEnableExecuteDisableFeature ( >>>  +  VOID >>>  +  ) >> >> I think we're over-complicating the name of this function. First, "To" >> looks unnecessary. Second, "Enable Execute Disable" is just an >> engineer's way to say "Disable Execution". Can we say right that: >> DisableExec()? >> >> Or at least, if we consider "NX" a word in its own right, "EnableNX()"? > > I prefer more general one. Let's use DisableExec(). > >> >>>  +{ >>>  +  if (!IsExecuteDisableBitAvailable ()) { >>>  +    return FALSE; >>>  +  } >>>  + >>>  +  // >>>  +  // Normally stack is type of EfiBootServicesData. Disabling NX  >>> for stack >>>  +  // but enabling NX for EfiBootServicesData doesn't make any sense. >>>  +  // >> >> This comment is good. >> >>>  +  if (PcdGetBool (PcdSetNxForStack) == FALSE && >> >> Please don't compare PcdGetBool() against TRUE or FALSE, just say >> PcdGetBool(), or !PcdGetBool(). >> >>>  +      (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &  >>> STACK_MEMORY_TYPE) >>  != 0) { >>>  +    DEBUG ((DEBUG_ERROR, >>>  +            "ERROR: NX for stack is disabled but NX for its memory  >>> type is enabled >> !\r\n")); >>>  +    ASSERT(!(PcdGetBool (PcdSetNxForStack) == FALSE && >>>  +             (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &  >>> STACK_MEMORY_T >> YPE) != 0)); >>>  +  } >> >> Please drop both the explicit "if", and the DEBUG message. Just keep  >> the comment (which is already fine) and the ASSERT(). The ASSERT()  >> will tell people where to look, and the comment will explain the  >> assertion. Also, in a RELEASE build, the check should be eliminated  >> entirely, but that might not work for the explicit "if" (dependent on  >> compilers and/or fixed vs. dynamic PCDs). >> >> Furthermore, keeping the logical negation operator as the outermost >> operator makes the code a lot harder to read. It's much better to just >> assert what we actually require, which is: >> >>   (DxeNxMemoryProtectionPolicy covers BSD) --> SetNxForStack >> >> put differently, >> >>   NOT(DxeNxMemoryProtectionPolicy covers BSD) OR SetNxForStack >> >> in C: >> >>   ASSERT ( >>     (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & STACK_MEMORY_TYPE) == >> 0 || >>     PcdGetBool (PcdSetNxForStack) >>     ); >> > > I don't have strong opinions on these. So let's do it your way. > >>>  + >>>  +  // >>>  +  // Image data section could be type of EfiLoaderData,  >>> EfiBootServicesData >>>  +  // or EfiRuntimeServicesData. Disabling NX for image data but  >>> enabling NX >>>  +  // for any those memory types doesn't make any sense. >>>  +  // >> >> The comment is good, I just suggest extending it with the origin of  >> the >> image: "Disabling NX for image data (regardless of image origin) for  >> any those memory types ...". >> > > Sure. I'll add it. > >>>  +  if (PcdGet32 (PcdImageProtectionPolicy) == 0 && >>>  +      (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & IMAGE_DATA_MEMOR >> Y_TYPE) != 0) { >>>  +    DEBUG ((DEBUG_ERROR, >>>  +            "ERROR: NX for image data is disabled but NX for its  >>> memory type(s) is >> enabled!\r\n")); >>>  +    ASSERT (!(PcdGet32 (PcdImageProtectionPolicy) == 0 && >>>  +              (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) &  >>> IMAGE_DATA_ME >> MORY_TYPE) != 0)); >>>  +  } >> >> Summarizing my points from before, here we should have: >> >>   ASSERT ( >>     (PcdGet64 (PcdDxeNxMemoryProtectionPolicy) & IMAGE_DATA_MEMORY_TY >> PE) == 0 || >>     PcdGet32 (PcdImageProtectionPolicy) == 3 >>     ); >> >> That is, >> >> - If we disable DxeNxMemoryProtectionPolicy for all of EfiLoaderData, >>   EfiBootServicesData, and EfiRuntimeServicesData, then any >>   ImageProtectionPolicy is fine. >> >> - If we enable  DxeNxMemoryProtectionPolicy for any of EfiLoaderData, >>   EfiBootServicesData, and EfiRuntimeServicesData, then we require the >>   platform to set ImageProtectionPolicy regardless of image origin. >> >> Thanks >> Laszlo >> > > Good catch. I missed that part. Thanks. > >>>  + >>>  +  // >>>  +  // 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); >>>  +} >>>  + >>>   /** >>>     Enable Execute Disable Bit. >>> >>>  @@ -755,7 +805,10 @@ CreateIdentityMappingPageTables ( >>>     // >>>     EnablePageTableProtection ((UINTN)PageMap, TRUE); >>> >>>  -  if (PcdGetBool (PcdSetNxForStack)) { >>>  +  // >>>  +  // Set IA32_EFER.NXE if necessary. >>>  +  // >>>  +  if (ToEnableExecuteDisableFeature ()) { >>>       EnableExecuteDisableBit (); >>>     } >>> >>>  diff -- >> git a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h b/MdeModulePkg >> /Core/DxeIplPeim/X64/VirtualMemory.h >>>  index 85457ff937..9f152e6531 100644 >>>  --- a/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >>>  +++ b/MdeModulePkg/Core/DxeIplPeim/X64/VirtualMemory.h >>>  @@ -179,6 +179,39 @@ typedef struct { >>>     UINTN           FreePages; >>>   } PAGE_TABLE_POOL; >>> >>>  +// >>>  +// Bit field repsentations of some EFI_MEMORY_TYPE, for page table  >>> initializ >> ation. >>>  +// >>>  +#define STACK_MEMORY_TYPE           (1 << EfiBootServicesData)     >>> /* 0x10 */ >>>  +#define IMAGE_DATA_MEMORY_TYPE      ((1 << EfiLoaderData)       |  >>> /* 0x04 >> */\ >>>  +                                     (1 << EfiBootServicesData) |  >>> /* 0x10 */\ >>>  +                                     (1 <<  >>> EfiRuntimeServicesData)/* 0x40 */\ >>>  +                                    )                              >>> /* 0x54 */ >>>  + >>>  +/** >>>  +  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 >>>  +ToEnableExecuteDisableFeature ( >>>  +  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. >>> >>>