On 10/9/23 02:07, Taylor Beebe wrote:
> Use SetMemoryProtectionsLib to set the memory protections for
> the platform in both normal and PEI-less boot. The protections
> set are equivalent to the PCD settings and the ability to set
> NxForStack via QemuCfg is preserved. Once the transition to use
> SetMemoryProtectionsLib and GetMemoryProtectionsLib is complete
> in the rest of EDK2, the mechanics of setting protections in
> OvmfPkg will be updated and the memory protection PCDs will
> be deleted.
>
> Signed-off-by: Taylor Beebe <taylor.d.be...@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Jordan Justen <jordan.l.jus...@intel.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> ---
> OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c | 15 +++++++++++++--
> OvmfPkg/PlatformPei/Platform.c | 15 +++++++++++++--
> OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf | 3 +++
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> 4 files changed, 30 insertions(+), 4 deletions(-)
This should be two patches; the audiences for both modules
(PeilessStartupLib vs. PlatformPei) are nearly distinct. I surely don't
care for PeilessStartupLib.
>
> diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> index 1632a2317718..cf645aad3246 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
> @@ -14,10 +14,13 @@
> #include <Protocol/DebugSupport.h>
> #include <Library/TdxLib.h>
> #include <IndustryStandard/Tdx.h>
> +#include <Library/PcdLib.h>
> #include <Library/PrePiLib.h>
> #include <Library/PeilessStartupLib.h>
> #include <Library/PlatformInitLib.h>
> #include <Library/TdxHelperLib.h>
> +#include <Library/SetMemoryProtectionsLib.h>
> +#include <Library/QemuFwCfgSimpleParserLib.h>
> #include <ConfidentialComputingGuestAttr.h>
> #include <Guid/MemoryTypeInformation.h>
> #include <OvmfPlatforms.h>
> @@ -42,7 +45,9 @@ InitializePlatform (
> EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> )
> {
> - VOID *VariableStore;
> + VOID *VariableStore;
> + DXE_MEMORY_PROTECTION_SETTINGS DxeSettings;
> + MM_MEMORY_PROTECTION_SETTINGS MmSettings;
>
> DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
> PlatformDebugDumpCmos ();
> @@ -104,7 +109,13 @@ InitializePlatform (
>
> PlatformMemMapInitialization (PlatformInfoHob);
>
> - PlatformNoexecDxeInitialization (PlatformInfoHob);
> + DxeSettings =
> DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsPcd].Settings;
> + MmSettings =
> MmMemoryProtectionProfiles[MmMemoryProtectionSettingsPcd].Settings;
> + DxeSettings.StackExecutionProtectionEnabled = PcdGetBool
> (PcdSetNxForStack);
> + QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack",
> &DxeSettings.StackExecutionProtectionEnabled);
> +
> + SetDxeMemoryProtectionSettings (&DxeSettings,
> DxeMemoryProtectionSettingsPcd);
> + SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
>
> if (TdIsEnabled ()) {
> PlatformInfoHob->PcdConfidentialComputingGuestAttr = CCAttrIntelTdx;
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index f5dc41c3a8c4..bcd8d3a1be14 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -38,6 +38,7 @@
> #include <IndustryStandard/QemuCpuHotplug.h>
> #include <Library/MemEncryptSevLib.h>
> #include <OvmfPlatforms.h>
> +#include <Library/SetMemoryProtectionsLib.h>
>
> #include "Platform.h"
>
> @@ -304,8 +305,10 @@ InitializePlatform (
> IN CONST EFI_PEI_SERVICES **PeiServices
> )
> {
> - EFI_HOB_PLATFORM_INFO *PlatformInfoHob;
> - EFI_STATUS Status;
> + EFI_HOB_PLATFORM_INFO *PlatformInfoHob;
> + EFI_STATUS Status;
> + DXE_MEMORY_PROTECTION_SETTINGS DxeSettings;
> + MM_MEMORY_PROTECTION_SETTINGS MmSettings;
>
> DEBUG ((DEBUG_INFO, "Platform PEIM Loaded\n"));
> PlatformInfoHob = BuildPlatformInfoHob ();
Can you check, in the OVMF debug log, the before-after temp stack usage?
My build of OVMF logs:
Temp Stack : BaseAddress=0x818000 Length=0x8000
Temp Heap : BaseAddress=0x810000 Length=0x8000
Total temporary memory: 65536 bytes.
temporary memory stack ever used: 30112 bytes.
temporary memory heap used for HobList: 7600 bytes.
I wonder if your change above makes a difference for stack usage here.
Point of interest: commit d41fd8e839a3 ("OvmfPkg: restore temporary
SEC/PEI RAM size to 64KB", 2017-11-17).
> @@ -342,6 +345,14 @@ InitializePlatform (
>
> PublishPeiMemory (PlatformInfoHob);
>
> + DxeSettings =
> DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsPcd].Settings;
> + MmSettings =
> MmMemoryProtectionProfiles[MmMemoryProtectionSettingsPcd].Settings;
Can you perhaps rearrange the source code somehow in order to prevent
uncrustify from turning these assignments into 110+ charater long lines?
But more importantly, these are structure assignments, and (I think)
structure assignments are (still) forbidden in edk2, because they can
produce calls to compiler-intrinsic helper functions.
So, please use CopyMem(). (And that should solve the line-too-long issue
as well!)
> + DxeSettings.StackExecutionProtectionEnabled = PcdGetBool
> (PcdSetNxForStack);
> + QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack",
> &DxeSettings.StackExecutionProtectionEnabled);
> +
> + SetDxeMemoryProtectionSettings (&DxeSettings,
> DxeMemoryProtectionSettingsPcd);
> + SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
> +
> PlatformQemuUc32BaseInitialization (PlatformInfoHob);
>
> InitializeRamRegions (PlatformInfoHob);
I don't like the placement of this logic (or minimally, I don't
understand it). This logic is supposed to be a superset of
NoexecDxeInitialization(), as the latter is what currently consumes the
"opt/ovmf/PcdSetNxForStack" fw_cfg file, and sets PcdSetNxForStack
accordingly.
But NoexecDxeInitialization() is restricted to
PlatformInfoHob->BootMode != BOOT_ON_S3_RESUME
whereas the new logic appears to run regardless of boot mode.
Did you test this series with ACPI S3 suspend/resume, both with and
without SMM_REQUIRE?
During S3 resume, there's not going to be a DXE phase, and so SMM is not
loaded yet again (it just survives from the first, cold, boot).
Therefore anything we expose (such as a HOB) that controls only DXE or
SMM is irrelevant during S3.
If there is at least one a setting in "DxeSettings" that *does* make a
difference even during S3 resume (stack guard?), then I'd argue that the
DXE settings are incorrectly structured / named.
Laszlo
> diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> index 585d50463748..f0a8a5a56df4 100644
> --- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> +++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf
> @@ -56,6 +56,8 @@ [LibraryClasses]
> PrePiLib
> QemuFwCfgLib
> PlatformInitLib
> + SetMemoryProtectionsLib
> + QemuFwCfgSimpleParserLib
>
> [Guids]
> gEfiHobMemoryAllocModuleGuid
> @@ -81,6 +83,7 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageProtectionPolicy ##
> SOMETIMES_CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask ##
> CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdNullPointerDetectionPropertyMask ##
> CONSUMES
> + gEfiMdeModulePkgTokenSpaceGuid.PcdSetNxForStack ##
> CONSUMES
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
> gUefiOvmfPkgTokenSpaceGuid.PcdSecureBootSupported
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 3934aeed9514..6b8442d12b2c 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -65,6 +65,7 @@ [LibraryClasses]
> PcdLib
> CcExitLib
> PlatformInitLib
> + SetMemoryProtectionsLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109443): https://edk2.groups.io/g/devel/message/109443
Mute This Topic: https://groups.io/mt/101843353/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
