On 11/05/15 03:34, Kinney, Michael D wrote: > Laszlo, > > Is there any reason why the table is only zeroed if > PcdSmmSmramRequire is TRUE? Seems like you could always clear this > table.
My general approach with this series has been to touch the "traditional" build's behavior as little as possible. > Either way, for this OVMF change: > > Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> Thanks! > 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. The "zero it in the constructor" approach was my first one, and the agreement we reached ultimately was to do it this way instead. Please see the sub-thread of the v1 posting of this patch: http://thread.gmane.org/gmane.comp.bios.edk2.devel/329/focus=328 So this is "option 2" from that thread. Thank you! Laszlo > > 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
