Re: [PATCH v2 1/2] MdeModulePkg/MdeModulePkg.dec/.uni: clarify PCDs usage

["Zeng, Star" <star.zeng@intel.com>]

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 <star.zeng@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Jian J Wang <jian.j.wang@intel.com>
> ---
>   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.<BR><BR>
> +  #  If a bit is clear, nothing will be done to image code/data sections.<BR><BR>
>     #    BIT0       - Image from unknown device. <BR>
>     #    BIT1       - Image from firmware volume.<BR>
> +  #  <BR>
> +  #  Note: If a bit is cleared, the data section could be still non-executable if
> +  #  PcdDxeNxMemoryProtectionPolicy is enabled for EfiLoaderData, EfiBootServicesData
> +  #  and/or EfiRuntimeServicesData.<BR>
> +  #  <BR>
>     # @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.<BR><BR>
> -  #
> +  #  non-executable.<BR>
> +  #  If a bit is cleared, nothing will be done to associated type of memory.<BR>
> +  #  <BR>
>     # Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>
>     #  EfiReservedMemoryType          0x0001<BR>
>     #  EfiLoaderCode                  0x0002<BR>
> @@ -1890,8 +1896,12 @@
>     #  For the DxeIpl and the DxeCore are both X64, set NX for stack feature also require PcdDxeIplBuildPageTables be TRUE.<BR>
>     #  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.<BR>
> -  #   TRUE  - to set NX for stack.<BR>
> -  #   FALSE - Not to set NX for stack.<BR>
> +  #  <BR>
> +  #  Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for
> +  #  EfiBootServicesData.<BR>

How about adding this sentence to be after TRUE/FALSE statements below?

Anyway, Reviewed-by: Star Zeng <star.zeng@intel.com>.

Thanks,
Star

> +  #  <BR>
> +  #   TRUE  - Set NX for stack.<BR>
> +  #   FALSE - Do nothing for stack.<BR>
>     # @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.<BR>"
>                                                                                     "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.<BR>"
> -                                                                                  "TRUE  - to set NX for stack.<BR>"
> -                                                                                  "FALSE - Not to set NX for stack.<BR>"
> +                                                                                  "Note: If this PCD is set to FALSE, NX could be still applied to stack due to PcdDxeNxMemoryProtectionPolicy enabled for EfiBootServicesData.<BR>"
> +                                                                                  "TRUE  - Set NX for stack.<BR>"
> +                                                                                  "FALSE - Do nothing for stack.<BR>"
>   
>   #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.<BR><BR>\n"
> +                                                                                          "If a bit is clear, nothing will be done to image code/data sections.<BR><BR>\n"
>                                                                                             "BIT0       - Image from unknown device. <BR>\n"
>                                                                                             "BIT1       - Image from firmware volume.<BR>"
> +                                                                                          "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.<BR>"
>   
>   #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.<BR><BR>\n"
> +                                                                                                "non-executable.<BR>\n"
> +                                                                                                "If a bit is cleared, nothing will be done to associated type of memory.<BR><BR>\n"
>                                                                                                   "\n"
>                                                                                                   "Below is bit mask for this PCD: (Order is same as UEFI spec)<BR>\n"
>                                                                                                   "EfiReservedMemoryType          0x0001<BR>\n"
>