BaseExtractGuidedSectionLib uses a table at the static physical address
PcdGuidedExtractHandlerTableAddress, and modules that are linked against
BaseExtractGuidedSectionLib are expected to work together on that table.
Namely, some modules can register handlers for GUIDed sections, some other
modules can decode such sections with the pre-registered handlers. The
table carries persistent information between these modules.

BaseExtractGuidedSectionLib checks a table signature whenever it is used
(by whichever module that is linked against it), and at the first use
(identified by a signature mismatch) it initializes the table.

One of the module types that BaseExtractGuidedSectionLib can be used with
is SEC, if the SEC module in question runs with the platform's RAM already
available.

In such cases the question emerges whether the initial contents of the RAM
(ie. contents that predate the very first signature check) can be trusted.
Normally RAM starts out with all zeroes (leading to a signature mismatch
on the first check); however a malicious runtime OS can populate the area
with some payload, then force a warm platform reset or an S3
suspend-and-resume. In such cases the signature check in the SEC module
might not fire, and ExtractGuidedSectionDecode() might run code injected
by the runtime OS, as part of SEC (ie. with high privileges).

-D SMM_REQUIRE signals that the firmware should be secure against
tampering from a malicious OS, therefore we clear the handler table in
SEC, in such builds.

See also git commit ad43bc6b2e (SVN rev 15433) -- this patch secures the
(d) and (e) code paths examined in that commit. Furthermore, a
non-malicious runtime OS will observe no change in behavior; see case (c)
in said commit.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <ler...@redhat.com>
---

Notes:
    v2:
    - This implements option #2 from Jordan's feedback at
      <http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=1995>.
      Liming was okay with that option:
      <http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=2009>.
    
    - This patch replaces the following patches from the v1 series:
    
      [PATCH 03/58] MdePkg: BaseExtractGuidedSectionLib: allow forced reinit
                    of handler table
      <http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=328>
    
      [PATCH 04/58] OvmfPkg: set PcdBaseExtractGuidedSectionLibForceInit for
                    SEC on SMM_REQUIRE
      <http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=330>

 OvmfPkg/Sec/SecMain.inf |  5 ++++
 OvmfPkg/Sec/SecMain.c   | 28 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
index fce99fb..e4bbb33 100644
--- a/OvmfPkg/Sec/SecMain.inf
+++ b/OvmfPkg/Sec/SecMain.inf
@@ -67,3 +67,8 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
+  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
+  gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
+
+[FeaturePcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
index b7df549..a773b6d 100644
--- a/OvmfPkg/Sec/SecMain.c
+++ b/OvmfPkg/Sec/SecMain.c
@@ -698,8 +698,36 @@ SecCoreStartupWithStack (
   IA32_DESCRIPTOR             IdtDescriptor;
   UINT32                      Index;
 
+  //
+  // This code may be running on the S3 resume boot path, where RAM has been
+  // populated by the runtime guest OS. Guarantee the reinitialization of the
+  // BaseExtractGuidedSectionLib handler table, before the constructor of
+  // LzmaCustomDecompressLib calls into BaseExtractGuidedSectionLib, due to our
+  // explicit ProcessLibraryConstructorList() call below.
+  //
+  // We must not rely on any libraries before calling
+  // ProcessLibraryConstructorList(), thus we clear the table with an
+  // open-coded loop. (The FeaturePcdGet() and FixedPcdGetXX() macro
+  // invocations are preprocessed to constants.)
+  //
+  if (FeaturePcdGet (PcdSmmSmramRequire)) {
+    UINT8 *Table;
+
+    Table = (UINT8*)(UINTN)FixedPcdGet64 (PcdGuidedExtractHandlerTableAddress);
+    for (Index = 0;
+         Index < FixedPcdGet32 (PcdGuidedExtractHandlerTableSize);
+         ++Index) {
+      Table[Index] = 0;
+    }
+  }
+
   ProcessLibraryConstructorList (NULL, NULL);
 
+  if (FeaturePcdGet (PcdSmmSmramRequire)) {
+    DEBUG ((EFI_D_VERBOSE, "%a: cleared BaseExtractGuidedSectionLib table\n",
+      __FUNCTION__));
+  }
+
   DEBUG ((EFI_D_INFO,
     "SecCoreStartupWithStack(0x%x, 0x%x)\n",
     (UINT32)(UINTN)BootFv,
-- 
1.8.3.1


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Reply via email to