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.20; helo=mga02.intel.com; envelope-from=star.zeng@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 78C732114AD0C for ; Thu, 20 Sep 2018 22:49:54 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 20 Sep 2018 22:49:54 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.54,283,1534834800"; d="scan'208";a="74793916" Received: from shzintpr03.sh.intel.com (HELO [10.7.209.58]) ([10.239.4.100]) by orsmga007.jf.intel.com with ESMTP; 20 Sep 2018 22:49:37 -0700 To: Jian J Wang , edk2-devel@lists.01.org Cc: Ruiyu Ni , Jiewen Yao , Laszlo Ersek , star.zeng@intel.com References: <20180920060247.7764-1-jian.j.wang@intel.com> <20180920060247.7764-2-jian.j.wang@intel.com> From: "Zeng, Star" Message-ID: <7472a98d-68bf-f41f-e988-a2616af043b6@intel.com> Date: Fri, 21 Sep 2018 13:49:06 +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: <20180920060247.7764-2-jian.j.wang@intel.com> Subject: Re: [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage 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 05:49:54 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Jian, The clarifications are very good. There is a very superficial comment at below. On 2018/9/20 14:02, Jian J Wang wrote: >> v2 changes: >> Newly added patch to clarify PCDs usage. > > BZ#1116: https://bugzilla.tianocore.org/show_bug.cgi?id=1116 > > The usage of following PCDs described in MdeModulePkg.dec don't match > the implementation exactly. This patch updates related description in > both .dec and .uni files to avoid confusion in platform configuration. > > PcdSetNxForStack > PcdImageProtectionPolicy > PcdDxeNxMemoryProtectionPolicy > > The main change is at the statement on how to handle the FALSE or 0 > setting value in those PCDs. Current statement says the implementation > should unset or disable related features but in fact the related code > just do nothing (leave it to AS-IS). That means the result might be > disabled, or might be not. It depends on other features or platform > policy. > > For example, if one don't want to enforce NX onto stack memory, he/she > needs to set PcdSetNxForStack to FALSE as well as to clear BIT4 of > 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/MdeModulePkg.dec | 20 +++++++++++++++----- > MdeModulePkg/MdeModulePkg.uni | 13 +++++++++---- > 2 files changed, 24 insertions(+), 9 deletions(-) > > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index 74a699cbb7..6566b57fd4 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -1288,17 +1288,23 @@ > ## Set image protection policy. The policy is bitwise. > # If a bit is set, the image will be protected by DxeCore if it is aligned. > # The code section becomes read-only, and the data section becomes non-executable. > - # If a bit is clear, the image will not be protected.

> + # If a bit is clear, nothing will be done to image code/data sections.

> # BIT0 - Image from unknown device.
> # BIT1 - Image from firmware volume.
> + #
> + # Note: If a bit is cleared, the data section could be still non-executable if > + # PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData > + # and/or EfiRuntimeServicesData.
> + #
> # @Prompt Set image protection policy. > # @ValidRange 0x80000002 | 0x00000000 - 0x0000001F > gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy|0x00000002|UINT32|0x00001047 > > ## Set DXE memory protection policy. The policy is bitwise. > # If a bit is set, memory regions of the associated type will be mapped > - # non-executable.

> - # > + # non-executable.
> + # If a bit is cleared, nothing will be done to associated type of memory.
> + #
> # Below is bit mask for this PCD: (Order is same as UEFI spec)
> # EfiReservedMemoryType 0x0001
> # EfiLoaderCode 0x0002
> @@ -1890,8 +1896,12 @@ > # For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.
> # For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require > # IA32 PAE is supported and Execute Disable Bit is available.
> - # TRUE - to set NX for stack.
> - # FALSE - Not to set NX for stack.
> + #
> + # Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for > + # EfiBootServicesData.
How about adding this sentence to be after TRUE/FALSE statements below? Anyway, Reviewed-by: Star Zeng . Thanks, Star > + #
> + # TRUE - Set NX for stack.
> + # FALSE - Do nothing for stack.
> # @Prompt Set NX for stack. > gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack|FALSE|BOOLEAN|0x0001006f > > diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni > index 080b8a62c0..61befdc1e4 100644 > --- a/MdeModulePkg/MdeModulePkg.uni > +++ b/MdeModulePkg/MdeModulePkg.uni > @@ -345,8 +345,9 @@ > "For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.
" > "For the DxeIpl and the DxeCore are both IA32 (PcdDxeIplSwitchToLongMode is FALSE), set NX for stack feature also require" > "IA32 PAE is supported and Execute Disable Bit is available.
" > - "TRUE - to set NX for stack.
" > - "FALSE - Not to set NX for stack.
" > + "Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for EfiBootServicesData.
" > + "TRUE - Set NX for stack.
" > + "FALSE - Do nothing for stack.
" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdAcpiS3Enable_PROMPT #language en-US "ACPI S3 Enable" > > @@ -1098,15 +1099,19 @@ > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdImageProtectionPolicy_HELP #language en-US "Set image protection policy. The policy is bitwise.\n" > "If a bit is set, the image will be protected by DxeCore if it is aligned.\n" > "The code section becomes read-only, and the data section becomes non-executable.\n" > - "If a bit is clear, the image will not be protected.

\n" > + "If a bit is clear, nothing will be done to image code/data sections.

\n" > "BIT0 - Image from unknown device.
\n" > "BIT1 - Image from firmware volume.
" > + "Note: If a bit is cleared, the data section could be still non-executable if\n" > + "PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData\n" > + "and/or EfiRuntimeServicesData.
" > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_PROMPT #language en-US "Set DXE memory protection policy." > > #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdDxeNxMemoryProtectionPolicy_HELP #language en-US "Set DXE memory protection policy. The policy is bitwise.\n" > "If a bit is set, memory regions of the associated type will be mapped\n" > - "non-executable.

\n" > + "non-executable.
\n" > + "If a bit is cleared, nothing will be done to associated type of memory.

\n" > "\n" > "Below is bit mask for this PCD: (Order is same as UEFI spec)
\n" > "EfiReservedMemoryType 0x0001
\n" >