On 10/9/23 02:07, Taylor Beebe wrote:
> Now that the EDK2 tree uses GetMemoryProtectionsLib to query
> the platform memory protection settings, OvmfPkg can be updated
> to use QemuCfg to set the entire memory protection profile instead
> of just SetNxForStack.
both the commit message and the subject say QemuCfg; should be QemuFwCfg
perhaps.
>
> For example, the following will set the DXE memory protection to
> the RELEASE preset.
> -fw_cfg name=opt/org.tianocore/DxeMemoryProtectionProfile,string=release
>
> The following will set the MM memory protection to
> the RELEASE preset.
> -fw_cfg name=opt/org.tianocore/MmMemoryProtectionProfile,string=release
>
> For users of Stuart, DXE_MEMORY_PROTECTION_PROFILE=release and
> MM_MEMORY_PROTECTION_PROFILE=release are equivalent to the above
> examples.
The lowercase "release" above does not match "Release" from patch 20
(MdeModulePkg: Add Additional Profiles to SetMemoryProtectionsLib).
Which makes me notice that you used AsciiStr*i*Cmp() in patch 22.
... I think that's unfortunate; it guarantees that scripts passing in
protection profile names will use inconsistent casing, so grepping for profile
names will be limited (people will forget to grep case-insensitively). I'd
rather be strict with profile names.
>
> 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>
> Cc: Rebecca Cran <rebe...@bsdio.com>
> Cc: Peter Grehan <gre...@freebsd.org>
> Cc: Corvin Köhne <corv...@freebsd.org>
> ---
> OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c | 21 ++++++++-----
> OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c | 13 +-------
> OvmfPkg/Library/PlatformInitLib/Platform.c | 15 ---------
> OvmfPkg/PlatformPei/IntelTdx.c | 2 --
> OvmfPkg/PlatformPei/Platform.c | 33
> +++++++-------------
> OvmfPkg/TdxDxe/TdxDxe.c | 7 ++---
> OvmfPkg/Include/Library/PlatformInitLib.h | 13 --------
> OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf | 2 +-
> OvmfPkg/PlatformCI/PlatformBuildLib.py | 8 +++++
> OvmfPkg/PlatformPei/PlatformPei.inf | 1 +
> 10 files changed, 37 insertions(+), 78 deletions(-)
This patch is way too big IMO, not by line count, but by number of actions
taken.
[cut]
> diff --git a/OvmfPkg/Include/Library/PlatformInitLib.h
> b/OvmfPkg/Include/Library/PlatformInitLib.h
> index 57b18b94d9b8..b2468f206321 100644
> --- a/OvmfPkg/Include/Library/PlatformInitLib.h
> +++ b/OvmfPkg/Include/Library/PlatformInitLib.h
> @@ -32,7 +32,6 @@ typedef struct {
> UINT32 Uc32Base;
> UINT32 Uc32Size;
>
> - BOOLEAN PcdSetNxForStack;
> UINT64 PcdTdxSharedBitMask;
>
> UINT64 PcdPciMmio64Base;
> @@ -182,18 +181,6 @@ PlatformMemMapInitialization (
> IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> );
>
> -/**
> - * Fetch "opt/ovmf/PcdSetNxForStack" from QEMU
> - *
> - * @param Setting The pointer to the setting of
> "/opt/ovmf/PcdSetNxForStack".
> - * @return EFI_SUCCESS Successfully fetch the settings.
> - */
> -EFI_STATUS
> -EFIAPI
> -PlatformNoexecDxeInitialization (
> - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> - );
> -
> VOID
> EFIAPI
> PlatformMiscInitialization (
> diff --git a/OvmfPkg/Library/PlatformInitLib/Platform.c
> b/OvmfPkg/Library/PlatformInitLib/Platform.c
> index f48bf16ae300..bc9becc4016e 100644
> --- a/OvmfPkg/Library/PlatformInitLib/Platform.c
> +++ b/OvmfPkg/Library/PlatformInitLib/Platform.c
> @@ -249,21 +249,6 @@ PlatformMemMapInitialization (
> PlatformInfoHob->PcdPciIoSize = PciIoSize;
> }
>
> -/**
> - * Fetch "opt/ovmf/PcdSetNxForStack" from QEMU
> - *
> - * @param Setting The pointer to the setting of
> "/opt/ovmf/PcdSetNxForStack".
> - * @return EFI_SUCCESS Successfully fetch the settings.
> - */
> -EFI_STATUS
> -EFIAPI
> -PlatformNoexecDxeInitialization (
> - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> - )
> -{
> - return QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack",
> &PlatformInfoHob->PcdSetNxForStack);
> -}
> -
> VOID
> PciExBarInitialization (
> VOID
One one hand: OK, these look consistent, they keep the HOB interface and the
lib header minimal and in sync.
However, removing the "opt/ovmf/PcdSetNxForStack" fw_cfg knob is a compat
breaking change. Do we have a plan for announcing that somehow?
[cut]
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index bcd8d3a1be14..fa76ad2de202 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -39,6 +39,7 @@
> #include <Library/MemEncryptSevLib.h>
> #include <OvmfPlatforms.h>
> #include <Library/SetMemoryProtectionsLib.h>
> +#include <Library/MemoryProtectionConfigLib.h>
>
> #include "Platform.h"
>
> @@ -74,21 +75,6 @@ MemMapInitialization (
> ASSERT_RETURN_ERROR (PcdStatus);
> }
>
> -STATIC
> -VOID
> -NoexecDxeInitialization (
> - IN OUT EFI_HOB_PLATFORM_INFO *PlatformInfoHob
> - )
> -{
> - RETURN_STATUS Status;
> -
> - Status = PlatformNoexecDxeInitialization (PlatformInfoHob);
> - if (!RETURN_ERROR (Status)) {
> - Status = PcdSetBoolS (PcdSetNxForStack,
> PlatformInfoHob->PcdSetNxForStack);
> - ASSERT_RETURN_ERROR (Status);
> - }
> -}
> -
> static const UINT8 EmptyFdt[] = {
> 0xd0, 0x0d, 0xfe, 0xed, 0x00, 0x00, 0x00, 0x48,
> 0x00, 0x00, 0x00, 0x38, 0x00, 0x00, 0x00, 0x48,
OK
> @@ -345,13 +331,17 @@ InitializePlatform (
>
> PublishPeiMemory (PlatformInfoHob);
>
> - DxeSettings =
> DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsPcd].Settings;
> - MmSettings =
> MmMemoryProtectionProfiles[MmMemoryProtectionSettingsPcd].Settings;
> - DxeSettings.StackExecutionProtectionEnabled = PcdGetBool
> (PcdSetNxForStack);
> - QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack",
> &DxeSettings.StackExecutionProtectionEnabled);
> + if (EFI_ERROR (ParseFwCfgDxeMemoryProtectionSettings (&DxeSettings))) {
> + SetDxeMemoryProtectionSettings (NULL,
> DxeMemoryProtectionSettingsGrubCompat);
> + } else {
> + SetDxeMemoryProtectionSettings (&DxeSettings,
> DxeMemoryProtectionSettingsGrubCompat);
> + }
>
> - SetDxeMemoryProtectionSettings (&DxeSettings,
> DxeMemoryProtectionSettingsPcd);
> - SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
> + if (EFI_ERROR (ParseFwCfgMmMemoryProtectionSettings (&MmSettings))) {
> + SetMmMemoryProtectionSettings (NULL, MmMemoryProtectionSettingsOff);
> + } else {
> + SetMmMemoryProtectionSettings (&MmSettings,
> MmMemoryProtectionSettingsOff);
> + }
>
> PlatformQemuUc32BaseInitialization (PlatformInfoHob);
>
OVMF can be built without SMM_REQUIRE; I think setting an MM profile in that
case is wasteful (but minimally: confusing). What's more...
> @@ -365,7 +355,6 @@ InitializePlatform (
> PeiFvInitialization (PlatformInfoHob);
> MemTypeInfoInitialization (PlatformInfoHob);
> MemMapInitialization (PlatformInfoHob);
> - NoexecDxeInitialization (PlatformInfoHob);
> }
>
> InstallClearCacheCallback ();
... your patch too shows that the new logic isn't truly compatible, or isn't
*compatibly placed*, with the old logic; the old logic is restricted to non-S3
boot paths (in effect just the boot with full config boot path).
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6b8442d12b2c..fbaa6bdc8ee5 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -66,6 +66,7 @@ [LibraryClasses]
> CcExitLib
> PlatformInitLib
> SetMemoryProtectionsLib
> + MemoryProtectionConfigLib
>
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
Hmm. I would have expected the QemuFwCfgSimpleParserLib dependency to be
removed here.
However, I realize: in patch v5 11/28 ("OvmfPkg: Apply Memory Protections via
SetMemoryProtectionsLib"), you do *not* add QemuFwCfgSimpleParserLib to
"PlatformPei.inf", even though you introduce a new QemuFwCfgParseBool() call.
So it's only fair that, when you now remove the QemuFwCfgParseBool() call, you
don't remove the QemuFwCfgSimpleParserLib dependency either.
But that only highlights a preexistent problem: even before your patch set,
"OvmfPkg/PlatformPei" doesn't depend on QemuFwCfgSimpleParserLib! There isn't a
single QemuFwCfgParse*() call in it!
So that's a preexistent bug, namely from commit 96047b6663ab
("OvmfPkg/PlatformInitLib: Move functions to Platform.c", 2022-04-02). That was
when the last QemuFwCfgParse*() reference was removed from OvmfPkg/PlatformPei,
but the lib class header inclusion and the lib class dependency weren't cleaned
up. Splendid.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109449): https://edk2.groups.io/g/devel/message/109449
Mute This Topic: https://groups.io/mt/101843367/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
