From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by ml01.01.org (Postfix) with ESMTP id 1A89C1A1DED for ; Wed, 10 Aug 2016 20:07:05 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 10 Aug 2016 20:07:04 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,503,1464678000"; d="scan'208";a="747137498" Received: from shzintpr02.sh.intel.com (HELO [10.253.24.29]) ([10.239.4.160]) by FMSMGA003.fm.intel.com with ESMTP; 10 Aug 2016 20:07:03 -0700 To: Andrew Fish References: <776695E1-E864-42F4-A633-B86FB913A510@apple.com> <0C09AFA07DD0434D9E2A0C6AEB048310036A1BCF@shsmsx102.ccr.corp.intel.com> <33692B19-74EE-4C13-8AE3-1C92B74836C3@apple.com> <9e979fb5-52ac-efe4-480e-8670d8233a7c@intel.com> <50FCE63D-C671-481E-B321-B0D06356A871@apple.com> <0C09AFA07DD0434D9E2A0C6AEB048310036A2959@shsmsx102.ccr.corp.intel.com> <4F6AB174-5C49-4E59-98F9-6E4B57DA31BF@apple.com> Cc: Mike Kinney , edk2-devel , star.zeng@intel.com From: "Zeng, Star" Message-ID: <69cb494f-8db0-0fc5-21e1-2692f5f85750@intel.com> Date: Thu, 11 Aug 2016 11:06:32 +0800 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <4F6AB174-5C49-4E59-98F9-6E4B57DA31BF@apple.com> Subject: Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Aug 2016 03:07:05 -0000 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 2016/8/11 9:59, Andrew Fish wrote: > >> On Aug 10, 2016, at 6:53 PM, Zeng, Star wrote: >> >> If a DXE_RUNTIME_DRIVER driver just wants to report status code at boot phase and links to DxeReportStatusCodeLib, is it a bug? >> >> How about just to add notes for DxeReportStatusCodeLib like DxePcdLib.inf? >> > > Star, > > This is point about lack of rules around this. I think the difference is the PCD protocol is defined as boot service only. I guess you could argue that depending on how things are constructed you could access some (FixedAtBuild) PCD things at runtime. > > I don't think it is a good idea to produce a runtime service that does not work at runtime. Mike seems to agree. > > But I agree better rules about this would be helpful. Andrew, I see the point, and I also agree it is helpful with good rules. I just thought about a possible valid use case from user, and propose to reduce the negative impact. Adding notes like DxePcdLib.inf is a possible approach to state the limitation, and there are also similar notes in SecPeiDxeTimerLibCpu.inf and SecPeiDxeTimerLibUefiCpu.inf. Thanks, Star > > Thanks, > > Andrew Fish > >> DxePcdLib.inf: >> " >> # Note: A driver of type DXE_RUNTIME_DRIVER and DXE_SMM_DRIVER can only use this DxePcdLib >> # in their initialization without any issues to access Dynamic and DynamicEx PCD. They can't >> # access Dynamic and DynamicEx PCD in the implementation of runtime services and SMI handlers. >> # Because EFI_PCD_PROTOCOL is DXE protocol that is not aviable in OS runtime phase. >> " >> >> Thanks, >> Star >> -----Original Message----- >> From: afish@apple.com [mailto:afish@apple.com] >> Sent: Thursday, August 11, 2016 3:04 AM >> To: Kinney, Michael D >> Cc: Zeng, Star ; edk2-devel >> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. >> >> >>> On Aug 10, 2016, at 11:02 AM, Kinney, Michael D wrote: >>> >>> >>> >>>> -----Original Message----- >>>> From: afish@apple.com [mailto:afish@apple.com] >>>> Sent: Wednesday, August 10, 2016 10:35 AM >>>> To: Kinney, Michael D >>>> Cc: Zeng, Star ; edk2-devel >>>> >>>> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due >>>> to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. >>>> >>>> >>>>> On Aug 10, 2016, at 10:22 AM, Kinney, Michael D wrote: >>>>> >>>>> Hi Andrew, >>>>> >>>>> I am staring at the code. Something is not right here, but I am >>>>> concerned that there are use cases I am not considering yet. Here >>>>> is my analysis so far. >>>>> >>>>> Here are the lib instances I see: >>>>> (ignoring IntelFramework ones for the purposes of this discussion). >>>>> >>>>> MdeModulePkg\Library\DxeReportStatusCodeLib\DxeReportStatusCodeLib.inf(25): >>>>> LIBRARY_CLASS = ReportStatusCodeLib|DXE_CORE DXE_DRIVER >>>>> DXE_RUNTIME_DRIVER >>>> DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE >>>>> >>>>> MdeModulePkg\Library\PeiReportStatusCodeLib\PeiReportStatusCodeLib.inf(27): >>>>> LIBRARY_CLASS = ReportStatusCodeLib|SEC PEIM PEI_CORE >>>>> >>>>> >>>> MdeModulePkg\Library\RuntimeDxeReportStatusCodeLib\RuntimeDxeReportSt >>>> atusCodeLib.inf( >>>> 23): >>>>> LIBRARY_CLASS = ReportStatusCodeLib|DXE_RUNTIME_DRIVER >>>>> DXE_SAL_DRIVER >>>>> >>>>> MdeModulePkg\Library\SmmReportStatusCodeLib\SmmReportStatusCodeLib.inf(26): >>>>> LIBRARY_CLASS = ReportStatusCodeLib|DXE_SMM_DRIVER SMM_CORE >>>>> >>>>> MdePkg\Library\BaseReportStatusCodeLibNull\BaseReportStatusCodeLibNull.inf(23): >>>>> LIBRARY_CLASS = ReportStatusCodeLib >>>>> >>>>> * The BASE one that is a Null instance makes sense when disabling >>>>> Report Status >>>> Code. >>>>> * The PEI one makes sense for its module compatibility. >>>>> * The SMM one also makes sense its module compatibility. >>>>> * And the RuntimeDxe one also makes sense its module compatibility. >>>>> * The DXE one seems to be over specified and if we reduced its >>>>> module compatibility, the SMM and RuntimeDxe lib instances can be >>>>> used to cover those module types. >>>>> >>>>> When I look at the source code in DxeReportStatusCodeLib, I see >>>>> comments that describe the use case where a module is dispatched >>>>> before the Report Status Code Protocol is available. >>>>> >>>>> The use case you are describing is when the first call to Report >>>>> Status Code by a module occurs after ExitBootServices(). In that >>>>> case, we depend on the Boot Services table being zeroed to exit early. >>>>> >>>>> I agree that no component should depend on Boot Services table being >>>>> cleared to zeros at ExitBootServices(). It is legal from UEFI/PI >>>>> spec to not zero at all, or in your example, fill with a pattern >>>>> other than zeros. >>>>> >>>>> It appears that the use case of first call to Report Status Code >>>>> after >>>>> ExitBootServices() is not covered by the DXE lib instance, and we >>>>> have just been getting lucky due to zeroing behavior of boot services table. >>>>> >>>>> I can think of a couple options: >>>>> >>>>> * Update DXE INF to remove the DXE Runtime, DXE SAL, SMM, SMM Core >>>>> module types. This may break platform builds if a platform is using >>>>> this lib mapping for modules of the types being removed. However, >>>>> those could be considered platform DSC bugs. >>>>> >>>>> * Update DXE lib instance to add ExitBootServices() and >>>>> SetVirtualAddressMap() events. However, this would basically make >>>>> the DXE lib instance the same as the DXE Runtime instance and would >>>>> increase size of non-RT components. >>>>> >>>>> I would prefer the first option, but we would need to do some >>>>> testing to see how many platform DSC files break. >>>>> >>>> >>>> Mike, >>>> >>>> That seems to make the most sense, but I too was worried about compatibility. >>>> >>>> I tried adding the UefiRuntimeLib to just fail calls at Runtime, but >>>> that breaks the build as DXE_CORE does not support UefiRuntimeLib. >>>> >>>> I guess one option would be to only add the ExitBootServices event >>>> and fail calls at runtime? >>> >>> I agree this would remove the assumption that the boot services table is zeroed. >>> However, the only way this function can be called after >>> ExitBootServices() is if the module is DXE_RUNTIME_DRIVER, >>> DXE_SAL_DRIVER, or DXE_SMM_DRIVER. If a module is one of these types >>> and they call Report Status Code, then they would want the status code >>> to go out. The current behavior of this lib instance silently ignores >>> all status codes after ExitBootServices(). This silent filtering is >>> not good, and may be a good reason to move ahead with the first option and fix the platform DSC files. >>> >> >> Mike, >> >> I have to admit the assumption that it works at runtime is how I hit this issue in the first place so I'm fine with restricting the usage. >> >> I guess the rule is: >> 1) For BootServices only libraries it is OK to map as Runtime Services as there is an assumption that the calls will only be made at boot services time. >> 2) For RuntimeServices libraries they should always work. >> >> I think the issue was a RT library that was coded with 1) assumptions. I have to admit I don't know that we do a good job of documenting this kind of thing. >> >> It also brings up the point of maybe it would be better to list fewer things as SMM from a security audit point of view. Maybe it would be worthing it to add runtime checks that fail for things marked SMM that are in bucket 1)? >> >> Thanks, >> >> Andrew Fish >> >>>> >>>> Thanks, >>>> >>>> Andrew Fish >>>> >>>>> Mike >>>>> >>>>>> -----Original Message----- >>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>>>> Of Andrew Fish >>>>>> Sent: Tuesday, August 9, 2016 7:50 AM >>>>>> To: Zeng, Star >>>>>> Cc: edk2-devel >>>>>> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed >>>>>> due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. >>>>>> >>>>>> >>>>>>> On Aug 8, 2016, at 7:26 PM, Zeng, Star wrote: >>>>>>> >>>>>>> On 2016/8/9 10:07, Andrew Fish wrote: >>>>>>>> >>>>>>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star wrote: >>>>>>>>> >>>>>>>>> Andrew, >>>>>>>>> >>>>>>>>> Should MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib be >>>>>>>>> used for your case >>>>>> if there are really runtime status code reporting needed? >>>>>>>>> >>>>>>>> >>>>>>>> Star, >>>>>>>> >>>>>>>> If the Library instance does not fully support >>>>>>>> DXE_RUNTIME_DRIVER, why is it >>>>>> listed in the LIBRARY_CLASS as supported? >>>>>>> >>>>>>> This kind of library can support DXE_RUNTIME_DRIVER at boot time, >>>>>>> for example >>>>>> UefiHiiLib, UefiHiiServicesLib, UefiBootServicesTableLib, DxePcdLib and etc. >>>>>>> >>>>>> >>>>>> Star, >>>>>> >>>>>> I understand giving access to Boot Services resources to a Runtime >>>>>> Driver for its constructor. I think the difference here is the >>>>>> DxeReportStatusCodeLib is >>>> abstracting >>>>>> a runtime service, but not doing it in a way to really works >>>>>> properly at runtime >>>> in >>>>>> all cases. I would expect that a library abstraction a runtime >>>>>> feature would work >>>> at >>>>>> runtime. Which is why I ended up with a "bad" mapping in the first place. >>>>>> >>>>>> Thanks, >>>>>> >>>>>> Andrew Fish >>>>>> >>>>>>>> >>>>>>>> >>>>>> >>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/Dx >>>> eReportStatusCod >>>>>> eLib/DxeReportStatusCodeLib.inf#L25 >>>>>>>> >>>>>>>> LIBRARY_CLASS = ReportStatusCodeLib|DXE_CORE DXE_DRIVER >>>>>> DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION >>>>>> UEFI_DRIVER SMM_CORE >>>>>>>> >>>>>>>> Actually I tried to add the UefiRuntimeDriverLib and the build >>>>>>>> failed as >>>>>> UefiRuntimeDriverLib was not supported for the DXE_CORE type. Maybe >>>>>> the bug is >>>> this >>>>>> library instance lists DXE_RUNTIME_DRIVER, DXE_SAL_DRIVER and >>>>>> DXE_SMM_DRIVER when >>>> it >>>>>> has special case code to support DXE_CORE? Maybe this library is >>>>>> trying to do too many things? >>>>>>>> >>>>>>>> What ReportStatusCodeLib would you recommend to link with RuntimeDxe driver: >>>>>> >>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Runti >>>> meDxe/RuntimeDxe >>>>>> .inf >>>>>>> >>>>>>> [LibraryClasses.common.DXE_CORE] >>>>>>> >>>>>> >>>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >>>> ReportStatusCodeLib|portStatusCodeLi >>>>>> b.inf >>>>>>> >>>>>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>>>>>> >>>>>> >>>> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLi >>>> ReportStatusCodeLib|b/RuntimeDxeRepo >>>>>> rtStatusCodeLib.inf >>>>>>> >>>>>>> [LibraryClasses.common.UEFI_DRIVER] >>>>>>> >>>>>> >>>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >>>> ReportStatusCodeLib|portStatusCodeLi >>>>>> b.inf >>>>>>> >>>>>>> [LibraryClasses.common.DXE_DRIVER] >>>>>>> >>>>>> >>>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >>>> ReportStatusCodeLib|portStatusCodeLi >>>>>> b.inf >>>>>>> >>>>>>> [LibraryClasses.common.UEFI_APPLICATION] >>>>>>> >>>>>> >>>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >>>> ReportStatusCodeLib|portStatusCodeLi >>>>>> b.inf >>>>>>> >>>>>>> Thanks, >>>>>>> Star >>>>>>> >>>>>>>> >>>>>>>> Thanks, >>>>>>>> >>>>>>>> Andrew Fish >>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> Star >>>>>>>>> -----Original Message----- >>>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On >>>>>>>>> Behalf Of Andrew >>>>>> Fish >>>>>>>>> Sent: Tuesday, August 9, 2016 7:08 AM >>>>>>>>> To: edk2-devel >>>>>>>>> Subject: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed >>>>>>>>> due to >>>>>> DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. >>>>>>>>> >>>>>>>>> I was messing about with an ExitBootServices test that fills >>>>>>>>> boot services >>>> memory >>>>>> with 0xAFAFAFAFAFAFAFAF (It was Vincent's idea to use my Initials >>>>>> but it has the handy property of being a non-cononical address and >>>>>> causes on GP fault on X64) and >>>>>> SetVirtualAddressMap() started crashing. >>>>>>>>> >>>>>>>>> It looks like this code is assuming the 1st call to ReportStatus >>>>>>>>> code will not >>>>>> happen at runtime. This is not the case for the RuntimeDxe driver. >>>>>>>>> >>>>>> >>>> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/Dx >>>> eReportStatusCod >>>>>> eLib/ReportStatusCodeLib.c#L43 >>>>>>>>> VOID >>>>>>>>> InternalGetReportStatusCode ( >>>>>>>>> VOID >>>>>>>>> ) >>>>>>>>> { >>>>>>>>> EFI_STATUS Status; >>>>>>>>> >>>>>>>>> if (mReportStatusCodeLibStatusCodeProtocol != NULL) { return; } >>>>>>>>> >>>>>>>>> // >>>>>>>>> // Check gBS just in case ReportStatusCode is called before gBS is initialized. >>>>>>>>> // >>>>>>>>> if (gBS != NULL && gBS->LocateProtocol != NULL) { Status = >>>>>>>>> gBS->LocateProtocol (&gEfiStatusCodeRuntimeProtocolGuid, NULL, >>>> (VOID**) >>>>>> &mReportStatusCodeLibStatusCodeProtocol); >>>>>>>>> if (EFI_ERROR (Status)) { >>>>>>>>> mReportStatusCodeLibStatusCodeProtocol = NULL; } } } >>>>>>>>> >>>>>>>>> I'm guessing this seems to work due >>>>>> >>>> to:https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dx >>>> e/DxeMain/DxeMai >>>>>> n.c#L803 >>>>>>>>> >>>>>>>>> // >>>>>>>>> // Zero out the Boot Service Table // ZeroMem (gBS, sizeof >>>>>>>>> (EFI_BOOT_SERVICES)); >>>>>>>>> >>>>>>>>> >>>>>>>>> Thus if I'm looking at this code correctly it only looks like it >>>>>>>>> works at >>>> Runtime >>>>>> since it is depending on the value of a boot services memory buffer not changing. >>>>>> This is not a valid assumption as that code is owned by the caller >>>>>> of ExitBootServices, so it should be legal for my test to change the value. >>>>>>>>> >>>>>>>>> I wanted to get a few more eyes on this prior to filling a bug? >>>>>>>>> >>>>>>>>> Thanks, >>>>>>>>> >>>>>>>>> Andrew Fish >>>>>>>>> >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> 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 >>>>>> >>>>>> _______________________________________________ >>>>>> edk2-devel mailing list >>>>>> edk2-devel@lists.01.org >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel