From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by ml01.01.org (Postfix) with ESMTP id DCD111A1DF3 for ; Wed, 10 Aug 2016 10:22:48 -0700 (PDT) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga102.fm.intel.com with ESMTP; 10 Aug 2016 10:22:15 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,501,1464678000"; d="scan'208";a="746937841" Received: from orsmsx109.amr.corp.intel.com ([10.22.240.7]) by FMSMGA003.fm.intel.com with ESMTP; 10 Aug 2016 10:22:15 -0700 Received: from orsmsx157.amr.corp.intel.com (10.22.240.23) by ORSMSX109.amr.corp.intel.com (10.22.240.7) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 10 Aug 2016 10:22:14 -0700 Received: from orsmsx113.amr.corp.intel.com ([169.254.9.118]) by ORSMSX157.amr.corp.intel.com ([169.254.9.232]) with mapi id 14.03.0248.002; Wed, 10 Aug 2016 10:22:14 -0700 From: "Kinney, Michael D" To: Andrew Fish , "Zeng, Star" , "Kinney, Michael D" CC: edk2-devel Thread-Topic: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. Thread-Index: AQHR8cmxdHq3oCRTQUaalkOkLQPo86BASp6AgAAMmQCAAAVcgIAAz+MAgAFEWmA= Date: Wed, 10 Aug 2016 17:22:13 +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> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ctpclassification: CTP_IC x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiZmJlNWU5YmYtMzA3Ny00NTJlLTgyZTItZWZhN2M2NDdlOGU0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InZjU2lPbUg0eUk5cXN0Y3E4ZGtwaVhQM0RBQThmeGZWYytqZCtIeG9RNjQ9In0= x-originating-ip: [10.22.254.140] 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 17:22:49 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Andrew, I am staring at the code. Something is not right here, but I am=20 concerned that there are use cases I am not considering yet. Here is my analysis so far. Here are the lib instances I see:=20 (ignoring IntelFramework ones for the purposes of this discussion). MdeModulePkg\Library\DxeReportStatusCodeLib\DxeReportStatusCodeLib.inf(25):= =20 LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_CORE DXE_DRIVER DXE_RUNTIME_DRI= VER 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\RuntimeDxeReportStatusCo= deLib.inf(23): LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_RUNTIME_DRIVER DXE_SAL_DRIVER 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 Report St= atus 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=20 compatibility, the SMM and RuntimeDxe lib instances can be used to cover= =20 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=20 Code by a module occurs after ExitBootServices(). In that case, we=20 depend on the Boot Services table being zeroed to exit early. I agree that no component should depend on Boot Services table being=20 cleared to zeros at ExitBootServices(). It is legal from UEFI/PI spec=20 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 SetVirtualAddressMa= p() events. However, this would basically make the DXE lib instance the same as the DXE Runtime instance and would increase size of non-RT=20 components. I would prefer the first option, but we would need to do some testing to see how many platform DSC files break. Mike > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of An= drew 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 ru= ntime. >=20 >=20 > > 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 e= xample > UefiHiiLib, UefiHiiServicesLib, UefiBootServicesTableLib, DxePcdLib and e= tc. > > >=20 > Star, >=20 > 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 wo= uld work at > runtime. Which is why I ended up with a "bad" mapping in the first place. >=20 > Thanks, >=20 > Andrew Fish >=20 > >> > >> > https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeRep= ortStatusCod > eLib/DxeReportStatusCodeLib.inf#L25 > >> > >> LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_CORE DXE_DR= IVER > DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DR= IVER > 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 b= ug is this > library instance lists DXE_RUNTIME_DRIVER, DXE_SAL_DRIVER and DXE_SMM_DR= IVER when it > has special case code to support DXE_CORE? Maybe this library is trying t= o do too > many things? > >> > >> What ReportStatusCodeLib would you recommend to link with RuntimeDxe d= river: > 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 O= f 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 ru= ntime. > >>> > >>> I was messing about with an ExitBootServices test that fills boot ser= vices 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/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 in= itialized. > >>> // > >>> 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 work= s at Runtime > since it is depending on the value of a boot services memory buffer not c= hanging. > 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 >=20 > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel