* Re: [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove XD enable/disable logic
2019-11-11 5:35 ` [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: " Ni, Ray
@ 2019-11-11 10:23 ` Laszlo Ersek
2019-11-13 6:50 ` Dong, Eric
1 sibling, 0 replies; 6+ messages in thread
From: Laszlo Ersek @ 2019-11-11 10:23 UTC (permalink / raw)
To: Ray Ni, devel; +Cc: Eric Dong
On 11/11/19 06:35, Ray Ni wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2329
>
> XD (ExecutionDisable) feature, when turned on, allows page table
> entry BIT63 set to 1 indicating the memory pointed by the page table
> is disallowed to execute.
> DxeIpl::CreateIdentityMappingPageTables() enables the XD when CPU
> supports it.
> Later DxeCore modifies the page table to set the BIT63 to protect
> the stack/heap to disallow code execution in stack/heap.
>
> UefiCpuPkg/CpuCommonFeaturesLib enables/disables the XD feature
> according to PcdCpuFeaturesSetting.
> When XD is disabled, GP fault is generated immediately because some
> page entries have BIT63 set.
>
> To fix this issue, this patch removes the XD feature logic from
> UefiCpuPkg/CpuCommonFeaturesLib so the XD feature is only taken
> care of by DxeIpl.
>
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> .../CpuCommonFeaturesLib.c | 11 ---
> .../CpuCommonFeaturesLib.inf | 3 +-
> .../CpuCommonFeaturesLib/ExecuteDisable.c | 95 -------------------
> 3 files changed, 1 insertion(+), 108 deletions(-)
> delete mode 100644 UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
>
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> index 238632f88a..3ebd9392a9 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> @@ -66,17 +66,6 @@ CpuCommonFeaturesLibConstructor (
> );
> ASSERT_EFI_ERROR (Status);
> }
> - if (IsCpuFeatureSupported (CPU_FEATURE_XD)) {
> - Status = RegisterCpuFeature (
> - "Execute Disable",
> - NULL,
> - ExecuteDisableSupport,
> - ExecuteDisableInitialize,
> - CPU_FEATURE_XD,
> - CPU_FEATURE_END
> - );
> - ASSERT_EFI_ERROR (Status);
> - }
> if (IsCpuFeatureSupported (CPU_FEATURE_FASTSTRINGS)) {
> Status = RegisterCpuFeature (
> "FastStrings",
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> index 6347c8997d..7fbcd8da0e 100644
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +++ b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> @@ -4,7 +4,7 @@
> # This library registers CPU features defined in Intel(R) 64 and IA-32
> # Architectures Software Developer's Manual.
> #
> -# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent
> #
> @@ -34,7 +34,6 @@ [Sources]
> C1e.c
> ClockModulation.c
> Eist.c
> - ExecuteDisable.c
> FastStrings.c
> FeatureControl.c
> LimitCpuIdMaxval.c
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> deleted file mode 100644
> index 75ea16309d..0000000000
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> +++ /dev/null
> @@ -1,95 +0,0 @@
> -/** @file
> - Execute Disable feature.
> -
> - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> - SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include "CpuCommonFeatures.h"
> -
> -/**
> - Detects if Execute Disable feature supported on current processor.
> -
> - @param[in] ProcessorNumber The index of the CPU executing this function.
> - @param[in] CpuInfo A pointer to the REGISTER_CPU_FEATURE_INFORMATION
> - structure for the CPU executing this function.
> - @param[in] ConfigData A pointer to the configuration buffer returned
> - by CPU_FEATURE_GET_CONFIG_DATA. NULL if
> - CPU_FEATURE_GET_CONFIG_DATA was not provided in
> - RegisterCpuFeature().
> -
> - @retval TRUE Execute Disable feature is supported.
> - @retval FALSE Execute Disable feature is not supported.
> -
> - @note This service could be called by BSP/APs.
> -**/
> -BOOLEAN
> -EFIAPI
> -ExecuteDisableSupport (
> - IN UINTN ProcessorNumber,
> - IN REGISTER_CPU_FEATURE_INFORMATION *CpuInfo,
> - IN VOID *ConfigData OPTIONAL
> - )
> -{
> - UINT32 Eax;
> - CPUID_EXTENDED_CPU_SIG_EDX Edx;
> -
> - AsmCpuid (CPUID_EXTENDED_FUNCTION, &Eax, NULL, NULL, NULL);
> - if (Eax <= CPUID_EXTENDED_FUNCTION) {
> - //
> - // Extended CPUID functions are not supported on this processor.
> - //
> - return FALSE;
> - }
> -
> - AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL, &Edx.Uint32);
> - return (Edx.Bits.NX != 0);
> -}
> -
> -/**
> - Initializes Execute Disable feature to specific state.
> -
> - @param[in] ProcessorNumber The index of the CPU executing this function.
> - @param[in] CpuInfo A pointer to the REGISTER_CPU_FEATURE_INFORMATION
> - structure for the CPU executing this function.
> - @param[in] ConfigData A pointer to the configuration buffer returned
> - by CPU_FEATURE_GET_CONFIG_DATA. NULL if
> - CPU_FEATURE_GET_CONFIG_DATA was not provided in
> - RegisterCpuFeature().
> - @param[in] State If TRUE, then the Execute Disable feature must be enabled.
> - If FALSE, then the Execute Disable feature must be disabled.
> -
> - @retval RETURN_SUCCESS Execute Disable feature is initialized.
> -
> - @note This service could be called by BSP only.
> -**/
> -RETURN_STATUS
> -EFIAPI
> -ExecuteDisableInitialize (
> - IN UINTN ProcessorNumber,
> - IN REGISTER_CPU_FEATURE_INFORMATION *CpuInfo,
> - IN VOID *ConfigData, OPTIONAL
> - IN BOOLEAN State
> - )
> -{
> - //
> - // The scope of the MSR_IA32_EFER is core for below processor type, only program
> - // MSR_IA32_EFER for thread 0 in each core.
> - //
> - if (IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily, CpuInfo->DisplayModel)) {
> - if (CpuInfo->ProcessorInfo.Location.Thread != 0) {
> - return RETURN_SUCCESS;
> - }
> - }
> -
> - CPU_REGISTER_TABLE_WRITE_FIELD (
> - ProcessorNumber,
> - Msr,
> - MSR_IA32_EFER,
> - MSR_IA32_EFER_REGISTER,
> - Bits.NXE,
> - (State) ? 1 : 0
> - );
> - return RETURN_SUCCESS;
> -}
>
series
Acked-by: Laszlo Ersek <lersek@redhat.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove XD enable/disable logic
2019-11-11 5:35 ` [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: " Ni, Ray
2019-11-11 10:23 ` Laszlo Ersek
@ 2019-11-13 6:50 ` Dong, Eric
1 sibling, 0 replies; 6+ messages in thread
From: Dong, Eric @ 2019-11-13 6:50 UTC (permalink / raw)
To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek
Reviewed-by: Eric Dong <eric.dong@intel.com>
> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Monday, November 11, 2019 11:05 AM
> To: devel@edk2.groups.io
> Cc: Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [PATCH 1/2] UefiCpuPkg/CpuCommonFeaturesLib: Remove XD
> enable/disable logic
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2329
>
> XD (ExecutionDisable) feature, when turned on, allows page table entry
> BIT63 set to 1 indicating the memory pointed by the page table is disallowed
> to execute.
> DxeIpl::CreateIdentityMappingPageTables() enables the XD when CPU
> supports it.
> Later DxeCore modifies the page table to set the BIT63 to protect the
> stack/heap to disallow code execution in stack/heap.
>
> UefiCpuPkg/CpuCommonFeaturesLib enables/disables the XD feature
> according to PcdCpuFeaturesSetting.
> When XD is disabled, GP fault is generated immediately because some page
> entries have BIT63 set.
>
> To fix this issue, this patch removes the XD feature logic from
> UefiCpuPkg/CpuCommonFeaturesLib so the XD feature is only taken care of
> by DxeIpl.
>
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
> .../CpuCommonFeaturesLib.c | 11 ---
> .../CpuCommonFeaturesLib.inf | 3 +-
> .../CpuCommonFeaturesLib/ExecuteDisable.c | 95 -------------------
> 3 files changed, 1 insertion(+), 108 deletions(-) delete mode 100644
> UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
>
> diff --git
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> index 238632f88a..3ebd9392a9 100644
> ---
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> +++
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.c
> @@ -66,17 +66,6 @@ CpuCommonFeaturesLibConstructor (
> );
> ASSERT_EFI_ERROR (Status);
> }
> - if (IsCpuFeatureSupported (CPU_FEATURE_XD)) {
> - Status = RegisterCpuFeature (
> - "Execute Disable",
> - NULL,
> - ExecuteDisableSupport,
> - ExecuteDisableInitialize,
> - CPU_FEATURE_XD,
> - CPU_FEATURE_END
> - );
> - ASSERT_EFI_ERROR (Status);
> - }
> if (IsCpuFeatureSupported (CPU_FEATURE_FASTSTRINGS)) {
> Status = RegisterCpuFeature (
> "FastStrings",
> diff --git
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> index 6347c8997d..7fbcd8da0e 100644
> ---
> a/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> +++
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
> @@ -4,7 +4,7 @@
> # This library registers CPU features defined in Intel(R) 64 and IA-32 #
> Architectures Software Developer's Manual.
> #
> -# Copyright (c) 2017 - 2018, Intel Corporation. All rights reserved.<BR>
> +# Copyright (c) 2017 - 2019, Intel Corporation. All rights
> +reserved.<BR>
> #
> # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -34,7 +34,6 @@
> [Sources]
> C1e.c
> ClockModulation.c
> Eist.c
> - ExecuteDisable.c
> FastStrings.c
> FeatureControl.c
> LimitCpuIdMaxval.c
> diff --git a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> b/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> deleted file mode 100644
> index 75ea16309d..0000000000
> --- a/UefiCpuPkg/Library/CpuCommonFeaturesLib/ExecuteDisable.c
> +++ /dev/null
> @@ -1,95 +0,0 @@
> -/** @file
> - Execute Disable feature.
> -
> - Copyright (c) 2017, Intel Corporation. All rights reserved.<BR>
> - SPDX-License-Identifier: BSD-2-Clause-Patent
> -
> -**/
> -
> -#include "CpuCommonFeatures.h"
> -
> -/**
> - Detects if Execute Disable feature supported on current processor.
> -
> - @param[in] ProcessorNumber The index of the CPU executing this
> function.
> - @param[in] CpuInfo A pointer to the
> REGISTER_CPU_FEATURE_INFORMATION
> - structure for the CPU executing this
> function.
> - @param[in] ConfigData A pointer to the configuration buffer
> returned
> - by
> CPU_FEATURE_GET_CONFIG_DATA. NULL if
> - CPU_FEATURE_GET_CONFIG_DATA
> was not provided in
> - RegisterCpuFeature().
> -
> - @retval TRUE Execute Disable feature is supported.
> - @retval FALSE Execute Disable feature is not supported.
> -
> - @note This service could be called by BSP/APs.
> -**/
> -BOOLEAN
> -EFIAPI
> -ExecuteDisableSupport (
> - IN UINTN ProcessorNumber,
> - IN REGISTER_CPU_FEATURE_INFORMATION *CpuInfo,
> - IN VOID *ConfigData OPTIONAL
> - )
> -{
> - UINT32 Eax;
> - CPUID_EXTENDED_CPU_SIG_EDX Edx;
> -
> - AsmCpuid (CPUID_EXTENDED_FUNCTION, &Eax, NULL, NULL, NULL);
> - if (Eax <= CPUID_EXTENDED_FUNCTION) {
> - //
> - // Extended CPUID functions are not supported on this processor.
> - //
> - return FALSE;
> - }
> -
> - AsmCpuid (CPUID_EXTENDED_CPU_SIG, NULL, NULL, NULL,
> &Edx.Uint32);
> - return (Edx.Bits.NX != 0);
> -}
> -
> -/**
> - Initializes Execute Disable feature to specific state.
> -
> - @param[in] ProcessorNumber The index of the CPU executing this
> function.
> - @param[in] CpuInfo A pointer to the
> REGISTER_CPU_FEATURE_INFORMATION
> - structure for the CPU executing this
> function.
> - @param[in] ConfigData A pointer to the configuration buffer
> returned
> - by
> CPU_FEATURE_GET_CONFIG_DATA. NULL if
> - CPU_FEATURE_GET_CONFIG_DATA
> was not provided in
> - RegisterCpuFeature().
> - @param[in] State If TRUE, then the Execute Disable
> feature must be enabled.
> - If FALSE, then the Execute Disable
> feature must be disabled.
> -
> - @retval RETURN_SUCCESS Execute Disable feature is initialized.
> -
> - @note This service could be called by BSP only.
> -**/
> -RETURN_STATUS
> -EFIAPI
> -ExecuteDisableInitialize (
> - IN UINTN ProcessorNumber,
> - IN REGISTER_CPU_FEATURE_INFORMATION *CpuInfo,
> - IN VOID *ConfigData, OPTIONAL
> - IN BOOLEAN State
> - )
> -{
> - //
> - // The scope of the MSR_IA32_EFER is core for below processor type,
> only program
> - // MSR_IA32_EFER for thread 0 in each core.
> - //
> - if (IS_SILVERMONT_PROCESSOR (CpuInfo->DisplayFamily,
> CpuInfo->DisplayModel)) {
> - if (CpuInfo->ProcessorInfo.Location.Thread != 0) {
> - return RETURN_SUCCESS;
> - }
> - }
> -
> - CPU_REGISTER_TABLE_WRITE_FIELD (
> - ProcessorNumber,
> - Msr,
> - MSR_IA32_EFER,
> - MSR_IA32_EFER_REGISTER,
> - Bits.NXE,
> - (State) ? 1 : 0
> - );
> - return RETURN_SUCCESS;
> -}
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 6+ messages in thread