Laszlo, Is there any reason why the table is only zeroed if PcdSmmSmramRequire is TRUE? Seems like you could always clear this table.
Either way, for this OVMF change: Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> I think there is a possibility we could move this logic into a constructor function for the BaseExtractGuidedSectionLib. If PcdGuidedExtractHandlerTableAddress is not 0 and PcdGuidedExtractHandlerTableSize is not 0, then the constructor could zero that buffer. Should still meet the requirement of zeroing before any BaseExtractGuidedSectionLib functions are used because build system orders the constructor calls based on dependencies. It would also require adding PcdGuidedExtractHandlerTableSize to MdePkg. Best regards, Mike >-----Original Message----- >From: Laszlo Ersek [mailto:ler...@redhat.com] >Sent: Tuesday, November 03, 2015 1:01 PM >To: edk2-de...@ml01.01.org >Cc: Kinney, Michael D; Justen, Jordan L >Subject: [PATCH v4 02/41] OvmfPkg: Sec: force reinit of >BaseExtractGuidedSectionLib handler table > >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. > >Cc: Michael Kinney <michael.d.kin...@intel.com> >Cc: Jordan Justen <jordan.l.jus...@intel.com> >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Laszlo Ersek <ler...@redhat.com> >[michael.d.kin...@intel.com: prevent VS20xx loop intrinsic with volatile] >Contributed-under: TianoCore Contribution Agreement 1.0 >Signed-off-by: Michael Kinney <michael.d.kin...@intel.com> >--- > >Notes: > v3: > - volatile-qualify the Table pointer to prevent VS20xx from turning the > zeroing loop into a compiler intrinsic [Mike] > > 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 2f78f3c..8f4d86c 100644 >--- a/OvmfPkg/Sec/SecMain.inf >+++ b/OvmfPkg/Sec/SecMain.inf >@@ -68,3 +68,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 4f87059..1556032 100644 >--- a/OvmfPkg/Sec/SecMain.c >+++ b/OvmfPkg/Sec/SecMain.c >@@ -699,8 +699,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)) { >+ volatile 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
