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. 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. > 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. Reviewed-by: Jordan Justen <[email protected]> > + > +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
