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 A4AC21A1E2B for ; Wed, 10 Aug 2016 11:02:46 -0700 (PDT) Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP; 10 Aug 2016 11:02:23 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,501,1464678000"; d="scan'208";a="1038797800" Received: from orsmsx105.amr.corp.intel.com ([10.22.225.132]) by fmsmga002.fm.intel.com with ESMTP; 10 Aug 2016 11:02:22 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.118]) by ORSMSX105.amr.corp.intel.com ([169.254.2.201]) with mapi id 14.03.0248.002; Wed, 10 Aug 2016 11:02:20 -0700 From: "Kinney, Michael D" To: "afish@apple.com" , "Kinney, Michael D" CC: "Zeng, Star" , edk2-devel Thread-Topic: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. Thread-Index: AQHR8cmxdHq3oCRTQUaalkOkLQPo86BASp6AgAAMmQCAAAVcgIAAz+MAgAFEWmCAAHwLAP//kN9g Date: Wed, 10 Aug 2016 18:02:20 +0000 Message-ID: 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> In-Reply-To: <50FCE63D-C671-481E-B321-B0D06356A871@apple.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMjdlMjQyZGEtOWEyMS00ODVmLTgyMTctYjE4MDM4NzBiMTczIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlN0Y3EzeTlyeVR6SGlzRWNMRFRpUWU1WUtxbjhVaEIwczFwQmdlOXVQSVU9In0= x-originating-ip: [10.22.254.139] MIME-Version: 1.0 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: Wed, 10 Aug 2016 18:02:46 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----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 ru= ntime. >=20 >=20 > > 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 =3D 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 =3D ReportStatusCodeLib|SEC PEIM PEI_CORE > > > > > MdeModulePkg\Library\RuntimeDxeReportStatusCodeLib\RuntimeDxeReportStatus= CodeLib.inf( > 23): > > LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVE= R > > > > MdeModulePkg\Library\SmmReportStatusCodeLib\SmmReportStatusCodeLib.inf(= 26): > > LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_SMM_DRIVER SMM_CORE > > > > MdePkg\Library\BaseReportStatusCodeLibNull\BaseReportStatusCodeLibNull.= inf(23): > > LIBRARY_CLASS =3D ReportStatusCodeLib > > > > * The BASE one that is a Null instance makes sense when disabling Repor= t 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 cov= er > > those module types. > > > > When I look at the source code in DxeReportStatusCodeLib, I see comment= s > > that describe the use case where a module is dispatched before the Repo= rt > > 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 modu= le > > 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 SetVirtualAddre= ssMap() > > 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 t= o > > see how many platform DSC files break. > > >=20 > Mike, >=20 > That seems to make the most sense, but I too was worried about compatibil= ity. >=20 > I tried adding the UefiRuntimeLib to just fail calls at Runtime, but that= breaks the > build as DXE_CORE does not support UefiRuntimeLib. >=20 > I guess one option would be to only add the ExitBootServices event and fa= il calls at > runtime? I agree this would remove the assumption that the boot services table is ze= roed. 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=20 module is one of these types and they call Report Status Code, then they wo= uld want the status code to go out. The current behavior of this lib instance silently ignores all status codes after ExitBootServices(). This silent fi= ltering is not good, and may be a good reason to move ahead with the first option a= nd fix the platform DSC files. >=20 > Thanks, >=20 > Andrew Fish >=20 > > 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 f= or your case > >> if there are really runtime status code reporting needed? > >>>>> > >>>> > >>>> Star, > >>>> > >>>> If the Library instance does not fully support DXE_RUNTIME_DRIVER, w= hy 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 an= d etc. > >>> > >> > >> Star, > >> > >> I understand giving access to Boot Services resources to a Runtime Dri= ver 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 pla= ce. > >> > >> Thanks, > >> > >> Andrew Fish > >> > >>>> > >>>> > >> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeRep= ortStatusCod > >> eLib/DxeReportStatusCodeLib.inf#L25 > >>>> > >>>> LIBRARY_CLASS =3D 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 faile= d as > >> UefiRuntimeDriverLib was not supported for the DXE_CORE type. Maybe th= e 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 tryin= g to do too > >> many things? > >>>> > >>>> What ReportStatusCodeLib would you recommend to link with RuntimeDxe= driver: > >> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/RuntimeDx= e/RuntimeDxe > >> .inf > >>> > >>> [LibraryClasses.common.DXE_CORE] > >>> > >> > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReport= StatusCodeLi > >> b.inf > >>> > >>> [LibraryClasses.common.DXE_RUNTIME_DRIVER] > >>> > >> > ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/Ru= ntimeDxeRepo > >> rtStatusCodeLib.inf > >>> > >>> [LibraryClasses.common.UEFI_DRIVER] > >>> > >> > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReport= StatusCodeLi > >> b.inf > >>> > >>> [LibraryClasses.common.DXE_DRIVER] > >>> > >> > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReport= StatusCodeLi > >> b.inf > >>> > >>> [LibraryClasses.common.UEFI_APPLICATION] > >>> > >> > ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReport= StatusCodeLi > >> 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 t= o > >> DxeReportStatusCodeLib assuming the state of the BootService Memory at= runtime. > >>>>> > >>>>> I was messing about with an ExitBootServices test that fills boot s= ervices > 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 co= de will not > >> happen at runtime. This is not the case for the RuntimeDxe driver. > >>>>> > >> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeRep= ortStatusCod > >> eLib/ReportStatusCodeLib.c#L43 > >>>>> VOID > >>>>> InternalGetReportStatusCode ( > >>>>> VOID > >>>>> ) > >>>>> { > >>>>> EFI_STATUS Status; > >>>>> > >>>>> if (mReportStatusCodeLibStatusCodeProtocol !=3D NULL) { > >>>>> return; > >>>>> } > >>>>> > >>>>> // > >>>>> // Check gBS just in case ReportStatusCode is called before gBS is = initialized. > >>>>> // > >>>>> if (gBS !=3D NULL && gBS->LocateProtocol !=3D NULL) { > >>>>> Status =3D gBS->LocateProtocol (&gEfiStatusCodeRuntimeProtocolGuid,= NULL, > (VOID**) > >> &mReportStatusCodeLibStatusCodeProtocol); > >>>>> if (EFI_ERROR (Status)) { > >>>>> mReportStatusCodeLibStatusCodeProtocol =3D NULL; > >>>>> } > >>>>> } > >>>>> } > >>>>> > >>>>> I'm guessing this seems to work due > >> > to:https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dxe/Dx= eMain/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 wo= rks at > Runtime > >> since it is depending on the value of a boot services memory buffer no= t 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 valu= e. > >>>>> > >>>>> 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