On 09/07/15 01:55, Jordan Justen wrote:
> 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.
Duly noted. Thank you for your flexibility. :)
>
>> 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"));
I agree. The "insecure" word looks needlessly alarming, and the
"hopefully secure" statement is a bit fishy too. Let's stick with the
facts. :) I'll update these.
> Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com>
Thank you!
Laszlo
>
>> 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
