Re: [edk2-devel] [PATCH v2 20/25] OvmfPkg: Enable Choosing Memory Protection Profile via QemuCfg

2023-08-29 Thread Taylor Beebe 



On 8/29/23 4:17 AM, Gerd Hoffmann wrote:

   Hi,


-  SetDxeMemoryProtectionSettings (&DxeSettings, 
DxeMemoryProtectionSettingsPcd);
-  SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
+  Status = QemuFwCfgParseString (DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, 
&StringSize, String);
+  if (!EFI_ERROR (Status)) {
+DEBUG ((DEBUG_INFO, "Setting DXE Memory Protection Profile: %a\n", 
String));
+if (AsciiStriCmp (String, "debug") == 0) {
+  DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;

I'd suggest to just loop over DxeMemoryProtectionProfiles and compare
String with .Name, so we don't have to touch this in case we add or
remove profiles.


Sounds good -- will update in v3


+DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;

I'd prefer to use DxeMemoryProtectionSettingsPcd by default.


The PCDs are still removed in this patch series. The PCD profile is 
included in the earlier patches of this series to ensure the memory 
protections are consistent as each patch transitions the references to 
use the library interface. I opted to to remove the PCDs for a couple of 
reasons:
1. The PCDs are the legacy interface, and keeping legacy interfaces 
around is sometimes necessary for compatibility but not in this case. 
Keeping the PCDs would disrupt maintainability, clarity, and 
extensibility of memory protections. I am also not confident the legacy 
interface would ever be removed in the future.


2. Removing the PCDs will cause a build failure for platforms which 
reference them. This outcome is desirable in this case because action 
needs to be taken to ensure the platform protection meets expectations 
with this new system. If the PCDs were kept, platform creators may try 
updating the PCDs and be confused when the changes are not reflected in 
the state of the system because the PCD profile is not in use. This 
nuance helps identify a confusing interface.




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108098): https://edk2.groups.io/g/devel/message/108098
Mute This Topic: https://groups.io/mt/100830924/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 20/25] OvmfPkg: Enable Choosing Memory Protection Profile via QemuCfg

2023-08-29 Thread Gerd Hoffmann 
  Hi,

> -  SetDxeMemoryProtectionSettings (&DxeSettings, 
> DxeMemoryProtectionSettingsPcd);
> -  SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
> +  Status = QemuFwCfgParseString (DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, 
> &StringSize, String);
> +  if (!EFI_ERROR (Status)) {
> +DEBUG ((DEBUG_INFO, "Setting DXE Memory Protection Profile: %a\n", 
> String));
> +if (AsciiStriCmp (String, "debug") == 0) {
> +  DxeSettings = 
> DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;

I'd suggest to just loop over DxeMemoryProtectionProfiles and compare
String with .Name, so we don't have to touch this in case we add or
remove profiles.

> +DxeSettings = 
> DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;

I'd prefer to use DxeMemoryProtectionSettingsPcd by default.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#108089): https://edk2.groups.io/g/devel/message/108089
Mute This Topic: https://groups.io/mt/100830924/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 20/25] OvmfPkg: Enable Choosing Memory Protection Profile via QemuCfg

2023-08-18 Thread Taylor Beebe 
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.

For example, the following will set the DXE memory protection to
the RELEASE preset. Other presets are "debug" and "off":
-fw_cfg name=opt/org.tianocore/DxeMemoryProtectionProfile,string=release

The following will set the DXE memory protection to
the RELEASE preset. Other presets are "debug" and "off":
-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.

Signed-off-by: Taylor Beebe 
Cc: Ard Biesheuvel 
Cc: Jiewen Yao 
Cc: Jordan Justen 
Cc: Gerd Hoffmann 
Cc: Rebecca Cran 
Cc: Peter Grehan 
Cc: Corvin Köhne 
---
 OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c   | 56 
++--
 OvmfPkg/Library/PeilessStartupLib/X64/VirtualMemory.c| 13 +---
 OvmfPkg/Library/PlatformInitLib/Platform.c   | 15 -
 OvmfPkg/Library/QemuFwCfgSimpleParserLib/QemuFwCfgSimpleParser.c | 11 +++
 OvmfPkg/PlatformPei/IntelTdx.c   |  2 -
 OvmfPkg/PlatformPei/Platform.c   | 70 
++--
 OvmfPkg/TdxDxe/TdxDxe.c  |  7 +-
 OvmfPkg/Bhyve/PlatformPei/PlatformPei.inf|  1 -
 OvmfPkg/Include/Library/PlatformInitLib.h| 13 
 OvmfPkg/Include/Library/QemuFwCfgSimpleParserLib.h   |  8 +++
 OvmfPkg/Library/PeilessStartupLib/PeilessStartupLib.inf  |  1 -
 OvmfPkg/PlatformCI/PlatformBuildLib.py   | 31 -
 OvmfPkg/PlatformPei/PlatformPei.inf  |  1 -
 OvmfPkg/TdxDxe/TdxDxe.inf|  1 -
 14 files changed, 148 insertions(+), 82 deletions(-)

diff --git a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c 
b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
index cf645aad3246..a6ac6a8a15cc 100644
--- a/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
+++ b/OvmfPkg/Library/PeilessStartupLib/PeilessStartup.c
@@ -28,6 +28,12 @@
 
 #define GET_GPAW_INIT_STATE(INFO)  ((UINT8) ((INFO) & 0x3f))
 
+#define DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \
+  "opt/org.tianocore/DxeMemoryProtectionProfile"
+
+#define MM_MEMORY_PROTECTION_PROFILE_FWCFG_FILE \
+  "opt/org.tianocore/MmMemoryProtectionProfile"
+
 EFI_MEMORY_TYPE_INFORMATION  mDefaultMemoryTypeInformation[] = {
   { EfiACPIMemoryNVS,   0x004 },
   { EfiACPIReclaimMemory,   0x008 },
@@ -48,6 +54,9 @@ InitializePlatform (
   VOID*VariableStore;
   DXE_MEMORY_PROTECTION_SETTINGS  DxeSettings;
   MM_MEMORY_PROTECTION_SETTINGS   MmSettings;
+  CHAR8   String[100];
+  UINTN   StringSize;
+  EFI_STATUS  Status;
 
   DEBUG ((DEBUG_INFO, "InitializePlatform in Pei-less boot\n"));
   PlatformDebugDumpCmos ();
@@ -109,18 +118,51 @@ InitializePlatform (
 
   PlatformMemMapInitialization (PlatformInfoHob);
 
-  DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsPcd].Settings;
-  MmSettings  = 
MmMemoryProtectionProfiles[MmMemoryProtectionSettingsPcd].Settings;
-  DxeSettings.StackExecutionProtectionEnabled = PcdGetBool (PcdSetNxForStack);
-  QemuFwCfgParseBool ("opt/ovmf/PcdSetNxForStack", 
&DxeSettings.StackExecutionProtectionEnabled);
+  StringSize = sizeof (String);
 
-  SetDxeMemoryProtectionSettings (&DxeSettings, 
DxeMemoryProtectionSettingsPcd);
-  SetMmMemoryProtectionSettings (&MmSettings, MmMemoryProtectionSettingsPcd);
+  Status = QemuFwCfgParseString (DXE_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, 
&StringSize, String);
+  if (!EFI_ERROR (Status)) {
+DEBUG ((DEBUG_INFO, "Setting DXE Memory Protection Profile: %a\n", 
String));
+if (AsciiStriCmp (String, "debug") == 0) {
+  DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;
+} else if (AsciiStriCmp (String, "release") == 0) {
+  DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsRelease].Settings;
+} else if (AsciiStriCmp (String, "off") == 0) {
+  DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsOff].Settings;
+} else {
+  DEBUG ((DEBUG_ERROR, "Invalid DXE memory protection profile: %a\n", 
String));
+  ASSERT (FALSE);
+}
+  } else {
+DxeSettings = 
DxeMemoryProtectionProfiles[DxeMemoryProtectionSettingsDebug].Settings;
+  }
+
+  Status = QemuFwCfgParseString (MM_MEMORY_PROTECTION_PROFILE_FWCFG_FILE, 
&StringSize, String);
+  if (!EFI_ERROR (Status)) {
+DEBUG ((DEBUG_INFO, "Setting MM Memory Protection Profile: %a\n", String));
+if (AsciiStriCmp (Strin