public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Michael Kubacki" <mikuback@linux.microsoft.com>
To: devel@edk2.groups.io
Cc: Chasel Chiu <chasel.chiu@intel.com>,
	Nate DeSimone <nathaniel.l.desimone@intel.com>,
	Isaac Oram <isaac.w.oram@intel.com>,
	Liming Gao <gaoliming@byosoft.com.cn>,
	Eric Dong <eric.dong@intel.com>,
	Ken Lautner <klautner@microsoft.com>
Subject: [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks
Date: Wed,  5 Jul 2023 21:57:26 -0400	[thread overview]
Message-ID: <20230706015726.269-1-mikuback@linux.microsoft.com> (raw)

From: Michael Kubacki <michael.kubacki@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@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Eric Dong <eric.dong@intel.com>
Co-authored-by: Ken Lautner <klautner@microsoft.com>
Signed-off-by: Michael Kubacki <michael.kubacki@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/PlatformInitPreMem.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


             reply	other threads:[~2023-07-06  1:57 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-06  1:57 Michael Kubacki [this message]
2023-07-06  2:57 ` [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add Mem Type Info variable validity checks Isaac Oram
     [not found] ` <176F27E11435C093.24383@groups.io>
2023-07-07  1:14   ` Isaac Oram

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230706015726.269-1-mikuback@linux.microsoft.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox