On 2015-07-24 16:00:12, Laszlo Ersek wrote:
> If OVMF was built with -D SMM_REQUIRE, that implies that the runtime OS is
> not trusted and we should defend against it tampering with the firmware's
> data.
>
> One such datum is the PEI firmware volume (PEIFV). Normally PEIFV is
> decompressed on the first boot by SEC, then the OS preserves it across S3
> suspend-resume cycles; at S3 resume SEC just reuses the originally
> decompressed PEIFV.
>
> However, if we don't trust the OS, then SEC must decompress PEIFV from the
> pristine flash every time, lest we execute OS-injected code or work with
> OS-injected data.
>
> Due to how FVMAIN_COMPACT is organized, we can't decompress just PEIFV;
> the decompression brings DXEFV with itself, plus it uses a temporary
> output buffer and a scratch buffer too, which even reach above the end of
> the finally installed DXEFV. For this reason we must keep away a
> non-malicious OS from DXEFV too, plus the memory up to
> PcdOvmfDecomprScratchEnd.
>
> The delay introduced by the LZMA decompression on S3 resume is negligible.
I definitely don't think this is the best we could do here. We
dicussed this a bit in irc back on April 23rd, and I was grasping for
some alternative implementations. :) Unfortuntately, none of the ideas
I had were nearly as easy as this, and thus this seems like a
reasonable next step.
> If -D SMM_REQUIRE is not specified, then PcdSmmSmramRequire remains FALSE
> (from the DEC file), and then this patch has no effect (not counting some
> changed debug messages).
>
> If QEMU doesn't support S3 (or the user disabled it on the QEMU command
> line), then this patch has no effect also.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
> ---
> OvmfPkg/PlatformPei/PlatformPei.inf | 4 +++
> OvmfPkg/Sec/SecMain.inf | 3 +++
> OvmfPkg/PlatformPei/Fv.c | 27 +++++++++++++++++++-
> OvmfPkg/PlatformPei/MemDetect.c | 11 +++++++-
> OvmfPkg/Sec/SecMain.c | 18 ++++++++++---
> 5 files changed, 58 insertions(+), 5 deletions(-)
>
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf
> b/OvmfPkg/PlatformPei/PlatformPei.inf
> index cb7d7dd..b9b0b20 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -75,6 +75,7 @@ [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
> gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecomprScratchEnd
> gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize
> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
> gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize
> @@ -87,6 +88,9 @@ [Pcd]
> gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
> gUefiCpuPkgTokenSpaceGuid.PcdCpuLocalApicBaseAddress
>
> +[FeaturePcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> +
> [Ppis]
> gEfiPeiMasterBootModePpiGuid
>
> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf
> index 0bd9f83..c66c7fc 100644
> --- a/OvmfPkg/Sec/SecMain.inf
> +++ b/OvmfPkg/Sec/SecMain.inf
> @@ -68,3 +68,6 @@ [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecomprScratchEnd
> +
> +[FeaturePcd]
> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
> diff --git a/OvmfPkg/PlatformPei/Fv.c b/OvmfPkg/PlatformPei/Fv.c
> index 3ed775c..7a89acd 100644
> --- a/OvmfPkg/PlatformPei/Fv.c
> +++ b/OvmfPkg/PlatformPei/Fv.c
> @@ -32,6 +32,8 @@ PeiFvInitialization (
> VOID
> )
> {
> + BOOLEAN SecureS3Needed;
> +
> DEBUG ((EFI_D_INFO, "Platform PEI Firmware Volume Initialization\n"));
>
> //
> @@ -50,16 +52,39 @@ PeiFvInitialization (
> //
> BuildFvHob (PcdGet32 (PcdOvmfDxeMemFvBase), PcdGet32
> (PcdOvmfDxeMemFvSize));
>
> + SecureS3Needed = mS3Supported && FeaturePcdGet (PcdSmmSmramRequire);
> +
> //
> // Create a memory allocation HOB for the DXE FV.
> //
> + // If "secure" S3 is needed, then SEC will decompress both PEI and DXE
> + // firmware volumes at S3 resume too, hence we need to keep away the OS
> from
> + // DXEFV as well. Otherwise we only need to keep away DXE itself from the
> + // DXEFV area.
> + //
> BuildMemoryAllocationHob (
> PcdGet32 (PcdOvmfDxeMemFvBase),
> PcdGet32 (PcdOvmfDxeMemFvSize),
> - EfiBootServicesData
> + SecureS3Needed ? EfiACPIMemoryNVS : EfiBootServicesData
> );
>
> //
> + // Additionally, said decompression will use temporary memory above the end
> + // of DXEFV, so let's keep away the OS from there too.
> + //
> + if (SecureS3Needed) {
> + UINT32 DxeMemFvEnd;
> +
> + DxeMemFvEnd = PcdGet32 (PcdOvmfDxeMemFvBase) +
> + PcdGet32 (PcdOvmfDxeMemFvSize);
> + BuildMemoryAllocationHob (
> + DxeMemFvEnd,
> + PcdGet32 (PcdOvmfDecomprScratchEnd) - DxeMemFvEnd,
> + EfiACPIMemoryNVS
> + );
> + }
> +
> + //
> // Let PEI know about the DXE FV so it can find the DXE Core
> //
> PeiServicesInstallFvInfoPpi (
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 612bb4a..649b484 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -222,7 +222,16 @@ PublishPeiMemory (
> //
> // Determine the range of memory to use during PEI
> //
> - MemoryBase = PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32
> (PcdOvmfDxeMemFvSize);
> + // Technically we could lay the permanent PEI RAM over SEC's temporary
> + // decompression and scratch buffer even if "secure S3" is needed, since
> + // their lifetimes don't overlap. However, PeiFvInitialization() will
> cover
> + // RAM up to PcdOvmfDecomprScratchEnd with an EfiACPIMemoryNVS memory
> + // allocation HOB, and other allocations served from the permanent PEI
> RAM
> + // shouldn't overlap with that HOB.
> + //
> + MemoryBase = mS3Supported && FeaturePcdGet (PcdSmmSmramRequire) ?
> + PcdGet32 (PcdOvmfDecomprScratchEnd) :
> + PcdGet32 (PcdOvmfDxeMemFvBase) + PcdGet32 (PcdOvmfDxeMemFvSize);
> MemorySize = LowerMemorySize - MemoryBase;
> if (MemorySize > PeiMemoryCap) {
> MemoryBase = LowerMemorySize - PeiMemoryCap;
> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c
> index 348d2b3..99dd6db 100644
> --- a/OvmfPkg/Sec/SecMain.c
> +++ b/OvmfPkg/Sec/SecMain.c
> @@ -538,13 +538,25 @@ FindPeiCoreImageBase (
> OUT EFI_PHYSICAL_ADDRESS *PeiCoreImageBase
> )
> {
> + BOOLEAN S3Resume;
> +
> *PeiCoreImageBase = 0;
>
> - if (IsS3Resume ()) {
> - DEBUG ((EFI_D_VERBOSE, "SEC: S3 resume\n"));
> + S3Resume = IsS3Resume ();
> + if (S3Resume && !FeaturePcdGet (PcdSmmSmramRequire)) {
> + //
> + // A malicious runtime OS may have injected something into our previously
> + // decoded PEI FV, but we don't care about that unless SMM/SMRAM is
> required.
> + //
> + DEBUG ((EFI_D_VERBOSE, "SEC: S3 resume (insecure)\n"));
How about instead:
DEBUG ((EFI_D_VERBOSE, "SEC: S3 resume\n"));
and
DEBUG ((EFI_D_VERBOSE, "SEC: S3 resume (with PEI decompression)\n"));
Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
> GetS3ResumePeiFv (BootFv);
> } else {
> - DEBUG ((EFI_D_VERBOSE, "SEC: Normal boot\n"));
> + //
> + // We're either not resuming, or resuming "securely" -- we'll decompress
> + // both PEI FV and DXE FV from pristine flash.
> + //
> + DEBUG ((EFI_D_VERBOSE, "SEC: %a\n",
> + S3Resume ? "S3 resume (hopefully secure)" : "Normal boot"));
> FindMainFv (BootFv);
>
> DecompressMemFvs (BootFv);
> --
> 1.8.3.1
>
>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
