Reviewed-by: Jordan Justen <[email protected]> On Tue, Apr 1, 2014 at 11:28 PM, Laszlo Ersek <[email protected]> wrote: > OVMF's SecMain is unique in the sense that it links against the following > two libraries *in combination*: > > - IntelFrameworkModulePkg/Library/LzmaCustomDecompressLib/ > LzmaCustomDecompressLib.inf > - MdePkg/Library/BaseExtractGuidedSectionLib/ > BaseExtractGuidedSectionLib.inf > > The ExtractGuidedSectionLib library class allows decompressor modules to > register themselves (keyed by GUID) with it, and it allows clients to > decompress file sections with a registered decompressor module that > matches the section's GUID. > > BaseExtractGuidedSectionLib is a library instance (of type BASE) for this > library class. It has no constructor function. > > LzmaCustomDecompressLib is a compatible decompressor module (of type > BASE). Its section type GUID is > > gLzmaCustomDecompressGuid == EE4E5898-3914-4259-9D6E-DC7BD79403CF > > When OVMF's SecMain module starts, the LzmaCustomDecompressLib constructor > function is executed, which registers its LZMA decompressor with the above > GUID, by calling into BaseExtractGuidedSectionLib: > > LzmaDecompressLibConstructor() [GuidedSectionExtraction.c] > ExtractGuidedSectionRegisterHandlers() [BaseExtractGuidedSectionLib.c] > GetExtractGuidedSectionHandlerInfo() > PcdGet64 (PcdGuidedExtractHandlerTableAddress) -- NOTE THIS > > Later, during a normal (non-S3) boot, SecMain utilizes this decompressor > to get information about, and to decompress, sections of the OVMF firmware > image: > > SecCoreStartupWithStack() [OvmfPkg/Sec/SecMain.c] > SecStartupPhase2() > FindAndReportEntryPoints() > FindPeiCoreImageBase() > DecompressMemFvs() > ExtractGuidedSectionGetInfo() [BaseExtractGuidedSectionLib.c] > ExtractGuidedSectionDecode() [BaseExtractGuidedSectionLib.c] > > Notably, only the extraction depends on full-config-boot; the registration > of LzmaCustomDecompressLib occurs unconditionally in the SecMain EFI > binary, triggered by the library constructor function. > > This is where the bug happens. BaseExtractGuidedSectionLib maintains the > table of GUIDed decompressors (section handlers) at a fixed memory > location; selected by PcdGuidedExtractHandlerTableAddress (declared in > MdePkg.dec). The default value of this PCD is 0x1000000 (16 MB). > > This causes SecMain to corrupt guest OS memory during S3, leading to > random crashes. Compare the following two memory dumps, the first taken > right before suspending, the second taken right after resuming a RHEL-7 > guest: > > crash> rd -8 -p 1000000 0x50 > 1000000: c0 00 08 00 02 00 00 00 00 00 00 00 00 00 00 00 ................ > 1000010: d0 33 0c 00 00 c9 ff ff c0 10 00 01 00 88 ff ff .3.............. > 1000020: 0a 6d 57 32 0f 00 00 00 38 00 00 01 00 88 ff ff .mW2....8....... > 1000030: 00 00 00 00 00 00 00 00 73 69 67 6e 61 6c 6d 6f ........signalmo > 1000040: 64 75 6c 65 2e 73 6f 00 00 00 00 00 00 00 00 00 dule.so......... > > vs. > > crash> rd -8 -p 1000000 0x50 > 1000000: 45 47 53 49 01 00 00 00 20 00 00 01 00 00 00 00 EGSI.... ....... > 1000010: 20 01 00 01 00 00 00 00 a0 01 00 01 00 00 00 00 ............... > 1000020: 98 58 4e ee 14 39 59 42 9d 6e dc 7b d7 94 03 cf .XN..9YB.n.{.... > 1000030: 00 00 00 00 00 00 00 00 73 69 67 6e 61 6c 6d 6f ........signalmo > 1000040: 64 75 6c 65 2e 73 6f 00 00 00 00 00 00 00 00 00 dule.so......... > > The "EGSI" signature corresponds to EXTRACT_HANDLER_INFO_SIGNATURE > declared in > MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.c. > > Additionally, the gLzmaCustomDecompressGuid (quoted above) is visible at > guest-phys offset 0x1000020. > > Fix the problem as follows: > - Carve out 4KB from the 36KB gap that we currently have between > > PcdOvmfLockBoxStorageBase + PcdOvmfLockBoxStorageSize == 8220 KB > and > PcdOvmfSecPeiTempRamBase == 8256 KB. > > - Point PcdGuidedExtractHandlerTableAddress to 8220 KB (0x00807000). > > - Cover the area with an EfiACPIMemoryNVS type memalloc HOB, if S3 is > supported and we're not currently resuming. > > The 4KB size that we pick is an upper estimate for > BaseExtractGuidedSectionLib's internal storage size. The latter is > calculated as follows (see GetExtractGuidedSectionHandlerInfo()): > > sizeof(EXTRACT_GUIDED_SECTION_HANDLER_INFO) + // 32 > PcdMaximumGuidedExtractHandler * ( > sizeof(GUID) + // 16 > sizeof(EXTRACT_GUIDED_SECTION_DECODE_HANDLER) + // 8 > sizeof(EXTRACT_GUIDED_SECTION_GET_INFO_HANDLER) // 8 > ) > > OVMF sets PcdMaximumGuidedExtractHandler to 16 decimal (which is the > MdePkg default too), yielding 32 + 16 * (16 + 8 + 8) == 544 bytes. > > Regarding the lifecycle of the new area: > > (a) when and how it is initialized after first boot of the VM > > The library linked into SecMain finds that the area lacks the signature. > It initializes the signature, plus the rest of the structure. This is > independent of S3 support. > > Consumption of the area is also limited to SEC (but consumption does > depend on full-config-boot). > > (b) how it is protected from memory allocations during DXE > > It is not, in the general case; and we don't need to. Nothing else links > against BaseExtractGuidedSectionLib; it's OK if DXE overwrites the area. > > (c) how it is protected from the OS > > When S3 is enabled, we cover it with AcpiNVS in InitializeRamRegions(). > > When S3 is not supported, the range is not protected. > > (d) how it is accessed on the S3 resume path > > Examined by the library linked into SecMain. Registrations update the > table in-place (based on GUID matches). > > (e) how it is accessed on the warm reset path > > If S3 is enabled, then the OS won't damage the table (due to (c)), hence > see (d). > > If S3 is unsupported, then the OS may or may not overwrite the > signature. (It likely will.) This is identical to the pre-patch status. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <[email protected]> > --- > OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++ > OvmfPkg/PlatformPei/MemDetect.c | 9 +++++++++ > OvmfPkg/OvmfPkg.dec | 1 + > OvmfPkg/OvmfPkgIa32.fdf | 3 +++ > OvmfPkg/OvmfPkgIa32X64.fdf | 3 +++ > OvmfPkg/OvmfPkgX64.fdf | 3 +++ > 6 files changed, 21 insertions(+) > > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf > b/OvmfPkg/PlatformPei/PlatformPei.inf > index 3b47bb7..a5fa9b5 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -72,7 +72,9 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > + gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize > + gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index 15b279e..4c22679 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -205,6 +205,15 @@ InitializeRamRegions ( > EfiACPIMemoryNVS > ); > > + // > + // SEC stores its table of GUIDed section handlers here. > + // > + BuildMemoryAllocationHob ( > + PcdGet64 (PcdGuidedExtractHandlerTableAddress), > + PcdGet32 (PcdGuidedExtractHandlerTableSize), > + EfiACPIMemoryNVS > + ); > + > #ifdef MDE_CPU_X64 > // > // Reserve the initial page tables built by the reset vector code. > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index aca9cb7..c10948d 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -88,6 +88,7 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdS3AcpiReservedMemoryBase|0x0|UINT32|0x17 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|0x0|UINT32|0x18 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize|0x0|UINT32|0x19 > + gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize|0x0|UINT32|0x1a > > [PcdsDynamic, PcdsDynamicEx] > gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index 202b81b..e661f8a 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -189,6 +189,9 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.P > 0x006000|0x001000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > > +0x007000|0x001000 > +gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index 0602880..5272b05 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -189,6 +189,9 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.P > 0x006000|0x001000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > > +0x007000|0x001000 > +gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 2e82d22..894d58f 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -189,6 +189,9 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.P > 0x006000|0x001000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > > +0x007000|0x001000 > +gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > -- > 1.8.3.1 > > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-devel mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/edk2-devel
------------------------------------------------------------------------------ _______________________________________________ edk2-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/edk2-devel
