Re: [edk2-devel] [PATCH v5 11/28] OvmfPkg: Apply Memory Protections via SetMemoryProtectionsLib

["Laszlo Ersek" <lersek@redhat.com>]

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.beebe@gmail.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Gerd Hoffmann <kraxel@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-