On 2015-09-05 23:43:27, Gao, Liming wrote: > Laszlo: > Yes. Current design supports to share GuidedTable pointed by > PcdGuidedExtractHandlerTableAddress between the different drivers.
This multiple image support was actually designed into the implementation? When would it be used? I think BaseExtractGuidedSectionLib is only useful in SEC phase, and SEC doesn't support multiple images, right? The PEI and DXE versions of the library support multiple images, and are a much better option if running in either PEI or DXE phases where multiple images are supported. I wouldn't want to see us add a PCD if it is not needed. I see two possible alternatives to this: 1. We decide BaseExtractGuidedSectionLib is only for SEC, and therefore we can always clear the signature in the constructor. 2. We require the platform to clear the memory region before it is used if they want the security property. For the non-virtual platforms that I'm familiar with, the setup for the pre-DRAM memory involves clearing the memory region, and therefore will be safe against this issue. For OVMF, we can easily read the address PCD and clear the memory early on in ROM code before the libraries are used. But, if the consensus is to just add the PCD, then I am not firmly against it. -Jordan > Your patch doesn't break this usage when PCD is configured to > FALSE. This change is OK to me. > > Thanks > Liming > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Laszlo > Ersek > Sent: Thursday, September 03, 2015 4:30 AM > To: Justen, Jordan L; edk2-de...@ml01.01.org; Gao, Liming; Kinney, Michael D > Subject: Re: [edk2] [PATCH 03/58] MdePkg: BaseExtractGuidedSectionLib: allow > forced reinit of handler table > > On 09/02/15 21:38, Jordan Justen wrote: > > On 2015-07-24 16:00:09, Laszlo Ersek wrote: > >> BaseExtractGuidedSectionLib uses a table at the static physical > >> address PcdGuidedExtractHandlerTableAddress, and modules that are > >> linked against BaseExtractGuidedSectionLib are expected to work together > >> on that table. > >> Namely, some modules can register handlers for GUIDed sections, some > >> other modules can decode such sections with the pre-registered > >> handlers. The table carries persistent information between these modules. > >> > >> BaseExtractGuidedSectionLib checks a table signature whenever it is > >> used (by whichever module that is linked against it), and at the > >> first use (identified by a signature mismatch) it initializes the table. > >> > >> One of the module types that BaseExtractGuidedSectionLib can be used > >> with is SEC, if the SEC module in question runs with the platform's > >> RAM already available. > > > > The implication seems to be that PcdGuidedExtractHandlerTableAddress > > might be able to point to pre-laid-out read-only data structure? I'm > > not sure, but I don't think BaseExtractGuidedSectionLib was designed > > with this in mind. I think PcdGuidedExtractHandlerTableAddress must > > always be writable memory. > > > > The other use-case that I know of besides OVMF (where RAM always > > works) is a pre-designated platform specific region of writeable > > memory that can be used before RAM is initialized. > > > > In other words, maybe it is always safe to nuke the signature in the > > constructor. Liming, Mike, what do you think? > > If it is safe to nuke the signature in the (new) constructor > *unconditionally*, then it's simpler to just extract the table init > code from GetExtractGuidedSectionHandlerInfo() into the new > constructor (similarly unconditionally). > > (I can't decide if this library instance was perhaps designed to > allow inter-module communication too, ie. when the library instance > is linked into separate modules, and the constructor would run > multiple times. Currently this doesn't happen, and OVMF certainly > doesn't need it (the only client is SecMain), but I couldn't exclude > it without documentation.) > > Thanks > Laszlo > > > > > -Jordan > > > >> In such cases the question emerges whether the initial contents of > >> the RAM (ie. contents that predate the very first signature check) can be > >> trusted. > >> Normally RAM starts out with all zeroes (leading to a signature > >> mismatch on the first check); however a malicious runtime OS can > >> populate the area with some payload, then force a warm platform reset > >> or an S3 suspend-and-resume. In such cases the signature check in the > >> SEC module might not fire, and ExtractGuidedSectionDecode() might run > >> code injected by the runtime OS, as part of SEC (ie. with high privileges). > >> > >> Allow DSC files to force specific linked instances of > >> BaseExtractGuidedSectionLib to reinitialize the table. > >> > >> Cc: Michael D Kinney <michael.d.kin...@intel.com> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> > >> Notes: > >> In the open source edk2 tree, no platform other than OvmfPkg uses > >> BaseExtractGuidedSectionLib, and OvmfPkg doesn't claim to be secure > >> against the described kind of attack (ie. code or data injection by a > >> malicious runtime OS before warm reset or S3) anyway. Hence as far as > >> the open source tree is concerned, the vulnerability is purely > >> theoretical. > >> > >> However, vendors might have closed source platforms that use > >> BaseExtractGuidedSectionLib in SEC, while also supporting Secure Boot. > >> For them the vulnerability might be real. > >> > >> In any case, I didn't try to come up with an actual exploit. > >> > >> I posted this patch to <tianocore-secur...@lists.sourceforge.net> on > >> Apr > >> 23rd, and haven't got any feedback since. So I'm including it here. > >> > >> > >> MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.inf > >> | 4 +++ > >> MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSectionLib.c > >> | 31 ++++++++++++++++++++ > >> MdePkg/MdePkg.dec > >> | 13 ++++++++ > >> 3 files changed, 48 insertions(+) > >> > >> diff --git > >> a/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.inf > >> b/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.inf > >> index 1bd245e..6fc528a 100644 > >> --- > >> a/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.inf > >> +++ b/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSec > >> +++ tionLib.inf > >> @@ -8,6 +8,7 @@ > >> # the implementation of runtime services, because this BASE library > >> instance doesn't # convert the address pointed by > >> PcdGuidedExtractHandlerTableAddress to the virtual address. > >> # > >> +# Copyright (C) 2015, Red Hat, Inc.<BR> > >> # Copyright (c) 2007 - 2014, Intel Corporation. All rights > >> reserved.<BR> # # This program and the accompanying materials @@ > >> -27,6 +28,7 @@ [Defines] > >> MODULE_TYPE = BASE > >> VERSION_STRING = 1.0 > >> LIBRARY_CLASS = ExtractGuidedSectionLib > >> + CONSTRUCTOR = BaseExtractGuidedSectionLibInit > >> > >> # > >> # The following information is for reference only and not required by the > >> build tools. > >> @@ -49,3 +51,5 @@ [Pcd] > >> gEfiMdePkgTokenSpaceGuid.PcdMaximumGuidedExtractHandler ## > >> CONSUMES > >> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress ## > >> CONSUMES > >> > >> +[FeaturePcd] > >> + gEfiMdePkgTokenSpaceGuid.PcdBaseExtractGuidedSectionLibForceInit > >> +## CONSUMES > >> diff --git > >> a/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.c > >> b/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.c > >> index 3a12cb1..d4dfb47 100644 > >> --- > >> a/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSection > >> Lib.c > >> +++ b/MdePkg/Library/BaseExtractGuidedSectionLib/BaseExtractGuidedSec > >> +++ tionLib.c > >> @@ -1,6 +1,7 @@ > >> /** @file > >> Provide generic extract guided section functions. > >> > >> + Copyright (C) 2015, Red Hat, Inc.<BR> > >> Copyright (c) 2007 - 2011, Intel Corporation. All rights reserved.<BR> > >> This program and the accompanying materials > >> are licensed and made available under the terms and conditions of > >> the BSD License @@ -30,6 +31,36 @@ typedef struct { } > >> EXTRACT_GUIDED_SECTION_HANDLER_INFO; > >> > >> /** > >> + The library constructor is generally a no-op. However, when linked > >> +into a SEC > >> + module that runs with RAM available, the platform description file > >> +should > >> + enable the constructor to force a table reinitialization in > >> + GetExtractGuidedSectionHandlerInfo() below, if the memory that SEC > >> +runs on is > >> + not trusted. > >> +**/ > >> +RETURN_STATUS > >> +EFIAPI > >> +BaseExtractGuidedSectionLibInit ( > >> + VOID > >> + ) > >> +{ > >> + EXTRACT_GUIDED_SECTION_HANDLER_INFO *HandlerInfo; > >> + > >> + if (!FeaturePcdGet (PcdBaseExtractGuidedSectionLibForceInit)) { > >> + return RETURN_SUCCESS; > >> + } > >> + > >> + HandlerInfo = (VOID*)(UINTN)PcdGet64 > >> + (PcdGuidedExtractHandlerTableAddress); > >> + if (HandlerInfo == NULL) { > >> + return EFI_OUT_OF_RESOURCES; > >> + } > >> + > >> + HandlerInfo->Signature = 0; > >> + DEBUG ((DEBUG_VERBOSE, "%a: cleared handler table signature\n", > >> + __FUNCTION__)); > >> + return RETURN_SUCCESS; > >> +} > >> + > >> +/** > >> HandlerInfo table address is set by > >> PcdGuidedExtractHandlerTableAddress, which is used to store > >> the registered guid and Handler list. When it is initialized, it will > >> be directly returned. > >> Or, HandlerInfo table will be initialized in this function. > >> diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec index > >> 598a6d0..5dc11b3 100644 > >> --- a/MdePkg/MdePkg.dec > >> +++ b/MdePkg/MdePkg.dec > >> @@ -1631,6 +1631,19 @@ [PcdsFeatureFlag] > >> # @Prompt Validate ORDERED_COLLECTION structure > >> > >> gEfiMdePkgTokenSpaceGuid.PcdValidateOrderedCollection|FALSE|BOOLEAN|0 > >> x0000002a > >> > >> + ## If TRUE, then the constructor of BaseExtractGuidedSectionLib > >> + will force a # reinitialization of the table of GUIDed section > >> + handlers, located at # PcdGuidedExtractHandlerTableAddress. This > >> + is useful when # BaseExtractGuidedSectionLib is linked into a SEC > >> + module that runs with RAM # ready, and the prior memory contents > >> + at # PcdGuidedExtractHandlerTableAddress are untrusted. > >> + # > >> + # The PCD should be left at FALSE generally, and overridden only > >> + for # individual SEC modules, with <PcdsFeatureFlag> stanzas in > >> + the [Components] # sections of DSC files. > >> + # @Prompt Force table initialization in > >> + BaseExtractGuidedSectionLib > >> + gEfiMdePkgTokenSpaceGuid.PcdBaseExtractGuidedSectionLibForceInit|FA > >> + LSE|BOOLEAN|0x0000002d > >> + > >> [PcdsFixedAtBuild] > >> ## Status code value for indicating a watchdog timer has expired. > >> # EFI_COMPUTING_UNIT_HOST_PROCESSOR | EFI_CU_HP_EC_TIMER_EXPIRED > >> -- > >> 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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
