On 09/06/15 21:56, Jordan Justen wrote: > On 2015-07-24 16:00:11, Laszlo Ersek wrote: >> The DecompressMemFvs() function in "OvmfPkg/Sec/SecMain.c" uses more >> memory, temporarily, than what PEIFV and DXEFV will ultimately need. >> First, it uses an output buffer for decompression, second, the >> decompression itself needs a scratch buffer (and this scratch buffer is >> the highest area that SEC uses). >> >> DecompressMemFvs() used to be called on normal boots only (ie. not on S3 >> resume), which is why the decompression output buffer and the scratch >> buffer were allowed to scribble over RAM. However, we'll soon start to >> worry during S3 resume that the runtime OS might tamper with the >> pre-decompressed PEIFV, and we'll decompress the firmware volumes on S3 >> resume too, from pristine flash. For this we'll need to know the end of >> the scratch buffer in advance, so we can prepare a non-malicious OS for >> it. >> >> Calculate the end of the scratch buffer statically in the FDF files, and >> assert in DecompressMemFvs() that the runtime decompression will match it. >> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Laszlo Ersek <[email protected]> >> --- >> OvmfPkg/Sec/SecMain.inf | 1 + >> OvmfPkg/Sec/SecMain.c | 8 +++ >> OvmfPkg/DecomprScratchEnd.fdf.inc | 72 ++++++++++++++++++++ >> OvmfPkg/OvmfPkg.dec | 1 + >> OvmfPkg/OvmfPkg.fdf.inc | 2 + >> OvmfPkg/OvmfPkgIa32.fdf | 4 +- >> OvmfPkg/OvmfPkgIa32X64.fdf | 4 +- >> OvmfPkg/OvmfPkgX64.fdf | 4 +- >> 8 files changed, 93 insertions(+), 3 deletions(-) >> >> diff --git a/OvmfPkg/Sec/SecMain.inf b/OvmfPkg/Sec/SecMain.inf >> index fce99fb..0bd9f83 100644 >> --- a/OvmfPkg/Sec/SecMain.inf >> +++ b/OvmfPkg/Sec/SecMain.inf >> @@ -67,3 +67,4 @@ [Pcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecomprScratchEnd > > I think we should just make Decompr => Decompression.
Okay, will update. > > It seems a bit odd to add the PCD for 'End' without 'Base'. Another > thought is to add PcdOvmfDecompressionBufferBase/End that covers the > entire block of RAM used for the decompression process. This is not a > big deal to me. I'd like to stick with "End only" -- the calculation works it way up from PcdOvmfDxeMemFvBase, so we already have a base address. > >> diff --git a/OvmfPkg/Sec/SecMain.c b/OvmfPkg/Sec/SecMain.c >> index 670ad8d..348d2b3 100644 >> --- a/OvmfPkg/Sec/SecMain.c >> +++ b/OvmfPkg/Sec/SecMain.c >> @@ -361,6 +361,14 @@ DecompressMemFvs ( >> >> OutputBuffer = (VOID*) ((UINT8*)(UINTN) PcdGet32 (PcdOvmfDxeMemFvBase) + >> SIZE_1MB); >> ScratchBuffer = ALIGN_POINTER ((UINT8*) OutputBuffer + OutputBufferSize, >> SIZE_1MB); >> + >> + DEBUG ((EFI_D_VERBOSE, "%a: OutputBuffer@%p+0x%x ScratchBuffer@%p+0x%x " >> + "PcdOvmfDecomprScratchEnd=0x%x\n", __FUNCTION__, OutputBuffer, >> + OutputBufferSize, ScratchBuffer, ScratchBufferSize, >> + PcdGet32 (PcdOvmfDecomprScratchEnd))); >> + ASSERT ((UINTN)ScratchBuffer + ScratchBufferSize == >> + PcdGet32 (PcdOvmfDecomprScratchEnd)); >> + >> Status = ExtractGuidedSectionDecode ( >> Section, >> &OutputBuffer, >> diff --git a/OvmfPkg/DecomprScratchEnd.fdf.inc >> b/OvmfPkg/DecomprScratchEnd.fdf.inc >> new file mode 100644 >> index 0000000..75df974 >> --- /dev/null >> +++ b/OvmfPkg/DecomprScratchEnd.fdf.inc >> @@ -0,0 +1,72 @@ >> +## @file >> +# This FDF include file computes the end of the scratch buffer used in >> +# DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. It is based on the >> decompressed >> +# (ie. original) size of the LZMA-compressed section of the one FFS file in >> +# the FVMAIN_COMPACT firmware volume. >> +# >> +# Copyright (C) 2015, Red Hat, Inc. >> +# >> +# This program and the accompanying materials are licensed and made >> available >> +# under the terms and conditions of the BSD License which accompanies this >> +# distribution. The full text of the license may be found at >> +# http://opensource.org/licenses/bsd-license.php >> +# >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, >> +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR >> +# IMPLIED. >> +## >> + >> +# The GUID EE4E5898-3914-4259-9D6E-DC7BD79403CF means >> "LzmaCustomDecompress". >> +# The decompressed output will have the following structure (see the file >> +# "9E21FD93-9C72-4c15-8C4B-E77F1DB2D792SEC1.guided.dummy" in the >> +# Build/Ovmf*/*/FV/Ffs/9E21FD93-9C72-4c15-8C4B-E77F1DB2D792/ directory): >> +# >> +# Size Contents >> +# ------------------- >> -------------------------------------------------------- >> +# 4 EFI_COMMON_SECTION_HEADER, stating size 124 (0x7C) >> and >> +# type 0x19 (EFI_SECTION_RAW). The purpose of this >> section >> +# is to pad the start of PEIFV to 128 bytes. >> +# 120 Zero bytes (padding). >> +# >> +# 4 EFI_COMMON_SECTION_HEADER, stating size >> +# (PcdOvmfPeiMemFvSize + 4), and type 0x17 >> +# (EFI_SECTION_FIRMWARE_VOLUME_IMAGE). >> +# PcdOvmfPeiMemFvSize PEIFV. Note that the above sizes pad the offset of >> this >> +# object to 128 bytes. See also the "guided.dummy.txt" >> +# file in the same directory. >> +# >> +# 4 EFI_COMMON_SECTION_HEADER, stating size 12 (0xC) and >> +# type 0x19 (EFI_SECTION_RAW). The purpose of this >> section >> +# is to pad the start of DXEFV to 16 bytes. >> +# 8 Zero bytes (padding). >> +# >> +# 4 EFI_COMMON_SECTION_HEADER, stating size >> +# (PcdOvmfDxeMemFvSize + 4), and type 0x17 >> +# (EFI_SECTION_FIRMWARE_VOLUME_IMAGE). >> +# PcdOvmfDxeMemFvSize DXEFV. Note that the above sizes pad the offset of >> this >> +# object to 16 bytes. See also the "guided.dummy.txt" >> file >> +# in the same directory. >> +# >> +# The total size after decompression is (128 + PcdOvmfPeiMemFvSize + 16 + >> +# PcdOvmfDxeMemFvSize). >> + >> +DEFINE OUTPUT_SIZE = (128 + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize >> + 16 + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize) >> + >> +# LzmaCustomDecompressLib uses a constant scratch buffer size of 64KB; see >> +# SCRATCH_BUFFER_REQUEST_SIZE in >> +# "MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaDecompress.c". >> + >> +DEFINE SCRATCH_SIZE = 0x00010000 >> + >> +# Note: when we use PcdOvmfDxeMemFvBase in this context, BaseTools have not >> yet >> +# offset it with MEMFD's base address. For that reason we have to do it >> manually. >> +# >> +# The calculation below mirrors DecompressMemFvs() [OvmfPkg/Sec/SecMain.c]. >> + >> +DEFINE OUTPUT_BASE = ($(MEMFD_BASE_ADDRESS) + >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase + 0x00100000) >> +DEFINE SCRATCH_BASE_UNALIGNED = ($(OUTPUT_BASE) + $(OUTPUT_SIZE)) >> +DEFINE SCRATCH_BASE_ALIGNMENT = 0x000FFFFF >> +DEFINE SCRATCH_BASE_MASK = 0xFFF00000 >> +DEFINE SCRATCH_BASE = (($(SCRATCH_BASE_UNALIGNED) + >> $(SCRATCH_BASE_ALIGNMENT)) & $(SCRATCH_BASE_MASK)) > > This 'SCRATCH' name usage is pretty localized, but it is a pretty > generic name. Maybe DECOMP_SCRATCH could be used instead. I think the > shorter 'DECOMP' is okay here since it is the only file where these > defines will be used. Once again, not really a big deal. I'll update it to DECOMP_SCRATCH. I'd like to keep SCRATCH as a connection to "OvmfPkg/Sec/SecMain.c". > Reviewed-by: Jordan Justen <[email protected]> Thank you! Laszlo > >> + >> +SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecomprScratchEnd = $(SCRATCH_BASE) + >> $(SCRATCH_SIZE) >> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec >> index 54df0ef..f9422f0 100644 >> --- a/OvmfPkg/OvmfPkg.dec >> +++ b/OvmfPkg/OvmfPkg.dec >> @@ -110,6 +110,7 @@ [PcdsFixedAtBuild] >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|0x0|UINT32|0x18 >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize|0x0|UINT32|0x19 >> >> gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize|0x0|UINT32|0x1a >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecomprScratchEnd|0x0|UINT32|0x1f >> >> [PcdsDynamic, PcdsDynamicEx] >> gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 >> diff --git a/OvmfPkg/OvmfPkg.fdf.inc b/OvmfPkg/OvmfPkg.fdf.inc >> index 486bbc6..441c35a 100644 >> --- a/OvmfPkg/OvmfPkg.fdf.inc >> +++ b/OvmfPkg/OvmfPkg.fdf.inc >> @@ -60,3 +60,5 @@ >> >> SET gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase = >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwWorkingBase + >> gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize >> SET gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize = 0x10000 >> + >> +DEFINE MEMFD_BASE_ADDRESS = 0x800000 >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf >> index 16675f8..efb4a4f 100644 >> --- a/OvmfPkg/OvmfPkgIa32.fdf >> +++ b/OvmfPkg/OvmfPkgIa32.fdf >> @@ -78,7 +78,7 @@ [FD.OVMF_CODE] >> >> ################################################################################ >> >> [FD.MEMFD] >> -BaseAddress = 0x800000 >> +BaseAddress = $(MEMFD_BASE_ADDRESS) >> Size = 0x900000 >> ErasePolarity = 1 >> BlockSize = 0x10000 >> @@ -377,6 +377,8 @@ [FV.FVMAIN_COMPACT] >> } >> } >> >> +!include DecomprScratchEnd.fdf.inc >> + >> >> ################################################################################ >> >> [Rule.Common.SEC] >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf >> index e6c525a..67c7b46 100644 >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf >> @@ -78,7 +78,7 @@ [FD.OVMF_CODE] >> >> ################################################################################ >> >> [FD.MEMFD] >> -BaseAddress = 0x800000 >> +BaseAddress = $(MEMFD_BASE_ADDRESS) >> Size = 0x900000 >> ErasePolarity = 1 >> BlockSize = 0x10000 >> @@ -377,6 +377,8 @@ [FV.FVMAIN_COMPACT] >> } >> } >> >> +!include DecomprScratchEnd.fdf.inc >> + >> >> ################################################################################ >> >> [Rule.Common.SEC] >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf >> index 08daf48..eec0559 100644 >> --- a/OvmfPkg/OvmfPkgX64.fdf >> +++ b/OvmfPkg/OvmfPkgX64.fdf >> @@ -78,7 +78,7 @@ [FD.OVMF_CODE] >> >> ################################################################################ >> >> [FD.MEMFD] >> -BaseAddress = 0x800000 >> +BaseAddress = $(MEMFD_BASE_ADDRESS) >> Size = 0x900000 >> ErasePolarity = 1 >> BlockSize = 0x10000 >> @@ -377,6 +377,8 @@ [FV.FVMAIN_COMPACT] >> } >> } >> >> +!include DecomprScratchEnd.fdf.inc >> + >> >> ################################################################################ >> >> [Rule.Common.SEC] >> -- >> 1.8.3.1 >> >> >> _______________________________________________ >> edk2-devel mailing list >> [email protected] >> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
