On 2015-11-03 13:00:38, Laszlo Ersek wrote:
> 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.)
> + //
I think this comment could be more terse. What about:
//
// To ensure SMM can't be compromised on S3 resume, we must force re-init
// of the BaseExtractGuidedSectionLib. Since this is before library
// contructors are called, we must use a loop rather than SetMem.
//
> + if (FeaturePcdGet (PcdSmmSmramRequire)) {
I agree with Mike. Let's just always zero the table.
> + 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__));
> + }
> +
And drop this.
Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> 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
