[edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks

[Isaac Oram]

Reviewed-by: Isaac Oram <isaac.w.oram at intel.com>

-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Wednesday, July 5, 2023 6:57 PM
To: devel at edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Oram, Isaac W <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>; Lautner, Kenneth <klautner at microsoft.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks

From: Michael Kubacki <michael.kubacki at microsoft.com>

Adds some sanity checks around the Memory Type Information data restored from the `EFI_MEMORY_TYPE_INFORMATION_VARIABLE_NAME` UEFI variable.

This is particularly useful when the structures that the data was saved against have changed in the latest firmware image. For example, `EfiUnacceptedMemoryType` was added to `EFI_MEMORY_TYPE` in
edk2 commit `502c01c`. This incremented `EfiMaxMemoryType` by `1`.

That change was first released in the `edk2-stable202211` stable tag.

Firmware performing an update across those stable tags may encounter issues depending on code implementation for handling `EfiMaxMemoryType` as a terminating loop value. This change checks the size and max memory type saved in the UEFI variable to determine whether it is better to start from the defaults and rebuild the UEFI variable data on the current boot.

Cc: Chasel Chiu <chasel.chiu at intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
Cc: Isaac Oram <isaac.w.oram at intel.com>
Cc: Liming Gao <gaoliming at byosoft.com.cn>
Cc: Eric Dong <eric.dong at intel.com>
Co-authored-by: Ken Lautner <klautner at microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki at microsoft.com>
---
 Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c | 32 +++++++++++++++++---
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
index d8c96b52f4b3..bc97711a02f6 100644
--- a/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/PlatformInitPreMem.c
+++ b/Platform/Intel/MinPlatformPkg/PlatformInit/PlatformInitPei/Platfor
+++ mInitPreMem.c
@@ -164,18 +164,22 @@ BuildMemoryTypeInformation (
   EFI_STATUS                      Status;
   EFI_PEI_READ_ONLY_VARIABLE2_PPI *VariableServices;
   UINTN                           DataSize;
+  UINTN                           Index;
   EFI_MEMORY_TYPE_INFORMATION     MemoryData[EfiMaxMemoryType + 1];
 
   //
   // Locate system configuration variable
   //
-  Status = PeiServicesLocatePpi(
+  Status = PeiServicesLocatePpi (
              &gEfiPeiReadOnlyVariable2PpiGuid, // GUID
              0,                                // INSTANCE
              NULL,                             // EFI_PEI_PPI_DESCRIPTOR
              (VOID **) &VariableServices       // PPI
              );
-  ASSERT_EFI_ERROR(Status);
+  if (EFI_ERROR (Status)) {
+    ASSERT_EFI_ERROR (Status);
+    return;
+  }
 
   DataSize = sizeof (MemoryData);
   Status = VariableServices->GetVariable ( @@ -186,9 +190,29 @@ BuildMemoryTypeInformation (
                                &DataSize,
                                &MemoryData
                                );
-  if (EFI_ERROR(Status)) {
+  if (!EFI_ERROR (Status)) {
+    if (DataSize % sizeof (EFI_MEMORY_TYPE_INFORMATION) != 0) {
+      DEBUG ((DEBUG_ERROR, "The UEFI Memory Type Information variable size is inconsistent with this build.\n"));
+      Status = EFI_COMPROMISED_DATA;
+    } else {
+      // Loop through all except the last one and make sure it seems reasonable
+      for (Index = 0; Index < ((DataSize / sizeof (EFI_MEMORY_TYPE_INFORMATION)) - 1); Index++) {
+        if (MemoryData[Index].Type >= EfiMaxMemoryType) {
+          DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable has an invalid memory type.\n"));
+          Status = EFI_COMPROMISED_DATA;
+        }
+      }
+      // The last entry must be MaxMemoryType with size 0
+      if ((MemoryData[Index].Type != EfiMaxMemoryType) || (MemoryData[Index].NumberOfPages != 0)) {
+        DEBUG ((DEBUG_ERROR, "UEFI Memory Type Information variable contains an invalid last entry.\n"));
+        Status = EFI_COMPROMISED_DATA;
+      }
+    }
+  }
+
+  if (EFI_ERROR (Status)) {
     DataSize = sizeof (mDefaultMemoryTypeInformation);
-    CopyMem(MemoryData, mDefaultMemoryTypeInformation, DataSize);
+    CopyMem (MemoryData, mDefaultMemoryTypeInformation, DataSize);
   }
 
   ///
--
2.41.0.windows.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106670): https://edk2.groups.io/g/devel/message/106670
Mute This Topic: https://groups.io/mt/99978201/1492418
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram at intel.com]
-=-=-=-=-=-=




-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#106676): https://edk2.groups.io/g/devel/message/106676
Mute This Topic: https://groups.io/mt/99978201/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-