From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by ml01.01.org (Postfix) with ESMTP id 9959F1A1DED for ; Wed, 10 Aug 2016 18:53:43 -0700 (PDT) Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga102.jf.intel.com with ESMTP; 10 Aug 2016 18:53:45 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,502,1464678000"; d="scan'208";a="1033525429" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by orsmga002.jf.intel.com with ESMTP; 10 Aug 2016 18:53:43 -0700 Received: from fmsmsx158.amr.corp.intel.com (10.18.116.75) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 10 Aug 2016 18:53:42 -0700 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by fmsmsx158.amr.corp.intel.com (10.18.116.75) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 10 Aug 2016 18:53:42 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.147]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.150]) with mapi id 14.03.0248.002; Thu, 11 Aug 2016 09:53:40 +0800 From: "Zeng, Star" To: "afish@apple.com" , "Kinney, Michael D" CC: edk2-devel , "Zeng, Star" Thread-Topic: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. Thread-Index: AQHR8cmugaYAxbIGqkqxNq+H8AFPWaA/1Png//+GyQCAAAVcgIAAz+MAgAG8xoCAAAOfAIAAB5cAgAARVwCAAPb04A== Date: Thu, 11 Aug 2016 01:53:40 +0000 Message-ID: <0C09AFA07DD0434D9E2A0C6AEB048310036A2959@shsmsx102.ccr.corp.intel.com> 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: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] 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: Thu, 11 Aug 2016 01:53:43 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable If a DXE_RUNTIME_DRIVER driver just wants to report status code at boot pha= se and links to DxeReportStatusCodeLib, is it a bug? How about just to add notes for DxeReportStatusCodeLib like DxePcdLib.inf? DxePcdLib.inf: " # Note: A driver of type DXE_RUNTIME_DRIVER and DXE_SMM_DRIVER can only use= this DxePcdLib=20 # in their initialization without any issues to access Dynamic and Dynamic= Ex PCD. They can't=20 # access Dynamic and DynamicEx PCD in the implementation of runtime servic= es and SMI handlers. # Because EFI_PCD_PROTOCOL is DXE protocol that is not aviable in OS runti= me phase. =20 " Thanks, Star -----Original Message----- From: afish@apple.com [mailto:afish@apple.com]=20 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 Dx= eReportStatusCodeLib assuming the state of the BootService Memory at runtim= e. > On Aug 10, 2016, at 11:02 AM, Kinney, Michael D wrote: >=20 >=20 >=20 >> -----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=20 >> >> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed due=20 >> to DxeReportStatusCodeLib assuming the state of the BootService Memory a= t runtime. >>=20 >>=20 >>> On Aug 10, 2016, at 10:22 AM, Kinney, Michael D wrote: >>>=20 >>> Hi Andrew, >>>=20 >>> 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=20 >>> is my analysis so far. >>>=20 >>> Here are the lib instances I see: >>> (ignoring IntelFramework ones for the purposes of this discussion). >>>=20 >>> MdeModulePkg\Library\DxeReportStatusCodeLib\DxeReportStatusCodeLib.inf(= 25): >>> LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_CORE DXE_DRIVER=20 >>> DXE_RUNTIME_DRIVER >> DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION UEFI_DRIVER SMM_CORE >>>=20 >>> MdeModulePkg\Library\PeiReportStatusCodeLib\PeiReportStatusCodeLib.inf(= 27): >>> LIBRARY_CLASS =3D ReportStatusCodeLib|SEC PEIM PEI_CORE >>>=20 >>>=20 >> MdeModulePkg\Library\RuntimeDxeReportStatusCodeLib\RuntimeDxeReportSt >> atusCodeLib.inf( >> 23): >>> LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_RUNTIME_DRIVER=20 >>> DXE_SAL_DRIVER >>>=20 >>> MdeModulePkg\Library\SmmReportStatusCodeLib\SmmReportStatusCodeLib.inf(= 26): >>> LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_SMM_DRIVER SMM_CORE >>>=20 >>> MdePkg\Library\BaseReportStatusCodeLibNull\BaseReportStatusCodeLibNull.= inf(23): >>> LIBRARY_CLASS =3D ReportStatusCodeLib >>>=20 >>> * The BASE one that is a Null instance makes sense when disabling=20 >>> 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=20 >>> module compatibility, the SMM and RuntimeDxe lib instances can be=20 >>> used to cover those module types. >>>=20 >>> When I look at the source code in DxeReportStatusCodeLib, I see=20 >>> comments that describe the use case where a module is dispatched=20 >>> before the Report Status Code Protocol is available. >>>=20 >>> The use case you are describing is when the first call to Report=20 >>> Status Code by a module occurs after ExitBootServices(). In that=20 >>> case, we depend on the Boot Services table being zeroed to exit early. >>>=20 >>> I agree that no component should depend on Boot Services table being=20 >>> cleared to zeros at ExitBootServices(). It is legal from UEFI/PI=20 >>> spec to not zero at all, or in your example, fill with a pattern=20 >>> other than zeros. >>>=20 >>> It appears that the use case of first call to Report Status Code=20 >>> after >>> ExitBootServices() is not covered by the DXE lib instance, and we=20 >>> have just been getting lucky due to zeroing behavior of boot services t= able. >>>=20 >>> I can think of a couple options: >>>=20 >>> * Update DXE INF to remove the DXE Runtime, DXE SAL, SMM, SMM Core=20 >>> module types. This may break platform builds if a platform is using=20 >>> this lib mapping for modules of the types being removed. However,=20 >>> those could be considered platform DSC bugs. >>>=20 >>> * Update DXE lib instance to add ExitBootServices() and=20 >>> SetVirtualAddressMap() events. However, this would basically make=20 >>> the DXE lib instance the same as the DXE Runtime instance and would=20 >>> increase size of non-RT components. >>>=20 >>> I would prefer the first option, but we would need to do some=20 >>> testing to see how many platform DSC files break. >>>=20 >>=20 >> Mike, >>=20 >> That seems to make the most sense, but I too was worried about compatibi= lity. >>=20 >> I tried adding the UefiRuntimeLib to just fail calls at Runtime, but=20 >> that breaks the build as DXE_CORE does not support UefiRuntimeLib. >>=20 >> I guess one option would be to only add the ExitBootServices event=20 >> and fail calls at runtime? >=20 > I agree this would remove the assumption that the boot services table is = zeroed. > However, the only way this function can be called after=20 > ExitBootServices() is if the module is DXE_RUNTIME_DRIVER,=20 > DXE_SAL_DRIVER, or DXE_SMM_DRIVER. If a module is one of these types=20 > and they call Report Status Code, then they would want the status code=20 > to go out. The current behavior of this lib instance silently ignores=20 > all status codes after ExitBootServices(). This silent filtering is=20 > not good, and may be a good reason to move ahead with the first option an= d fix the platform DSC files. >=20 Mike, I have to admit the assumption that it works at runtime is how I hit this i= ssue in the first place so I'm fine with restricting the usage.=20 I guess the rule is: 1) For BootServices only libraries it is OK to map as Runtime Services as t= here is an assumption that the calls will only be made at boot services tim= e. 2) For RuntimeServices libraries they should always work.=20 I think the issue was a RT library that was coded with 1) assumptions. I ha= ve to admit I don't know that we do a good job of documenting this kind of = thing.=20 It also brings up the point of maybe it would be better to list fewer thing= s 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= )?=20 Thanks, Andrew Fish >>=20 >> Thanks, >>=20 >> Andrew Fish >>=20 >>> Mike >>>=20 >>>> -----Original Message----- >>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf=20 >>>> Of Andrew Fish >>>> Sent: Tuesday, August 9, 2016 7:50 AM >>>> To: Zeng, Star >>>> Cc: edk2-devel >>>> Subject: Re: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed=20 >>>> due to DxeReportStatusCodeLib assuming the state of the BootService Me= mory at runtime. >>>>=20 >>>>=20 >>>>> On Aug 8, 2016, at 7:26 PM, Zeng, Star wrote: >>>>>=20 >>>>> On 2016/8/9 10:07, Andrew Fish wrote: >>>>>>=20 >>>>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star wrote: >>>>>>>=20 >>>>>>> Andrew, >>>>>>>=20 >>>>>>> Should MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib be=20 >>>>>>> used for your case >>>> if there are really runtime status code reporting needed? >>>>>>>=20 >>>>>>=20 >>>>>> Star, >>>>>>=20 >>>>>> If the Library instance does not fully support=20 >>>>>> DXE_RUNTIME_DRIVER, why is it >>>> listed in the LIBRARY_CLASS as supported? >>>>>=20 >>>>> This kind of library can support DXE_RUNTIME_DRIVER at boot time,=20 >>>>> for example >>>> UefiHiiLib, UefiHiiServicesLib, UefiBootServicesTableLib, DxePcdLib an= d etc. >>>>>=20 >>>>=20 >>>> Star, >>>>=20 >>>> I understand giving access to Boot Services resources to a Runtime=20 >>>> Driver for its constructor. I think the difference here is the=20 >>>> DxeReportStatusCodeLib is >> abstracting >>>> a runtime service, but not doing it in a way to really works=20 >>>> properly at runtime >> in >>>> all cases. I would expect that a library abstraction a runtime=20 >>>> feature would work >> at >>>> runtime. Which is why I ended up with a "bad" mapping in the first pla= ce. >>>>=20 >>>> Thanks, >>>>=20 >>>> Andrew Fish >>>>=20 >>>>>>=20 >>>>>>=20 >>>>=20 >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/Dx >> eReportStatusCod >>>> eLib/DxeReportStatusCodeLib.inf#L25 >>>>>>=20 >>>>>> LIBRARY_CLASS =3D ReportStatusCodeLib|DXE_CORE DXE_= DRIVER >>>> DXE_RUNTIME_DRIVER DXE_SAL_DRIVER DXE_SMM_DRIVER UEFI_APPLICATION=20 >>>> UEFI_DRIVER SMM_CORE >>>>>>=20 >>>>>> Actually I tried to add the UefiRuntimeDriverLib and the build=20 >>>>>> failed as >>>> UefiRuntimeDriverLib was not supported for the DXE_CORE type. Maybe=20 >>>> the bug is >> this >>>> library instance lists DXE_RUNTIME_DRIVER, DXE_SAL_DRIVER and=20 >>>> DXE_SMM_DRIVER when >> it >>>> has special case code to support DXE_CORE? Maybe this library is=20 >>>> trying to do too many things? >>>>>>=20 >>>>>> What ReportStatusCodeLib would you recommend to link with RuntimeDxe= driver: >>>>=20 >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Runti >> meDxe/RuntimeDxe >>>> .inf >>>>>=20 >>>>> [LibraryClasses.common.DXE_CORE] >>>>>=20 >>>>=20 >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >> ReportStatusCodeLib|portStatusCodeLi >>>> b.inf >>>>>=20 >>>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER] >>>>>=20 >>>>=20 >> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLi >> ReportStatusCodeLib|b/RuntimeDxeRepo >>>> rtStatusCodeLib.inf >>>>>=20 >>>>> [LibraryClasses.common.UEFI_DRIVER] >>>>>=20 >>>>=20 >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >> ReportStatusCodeLib|portStatusCodeLi >>>> b.inf >>>>>=20 >>>>> [LibraryClasses.common.DXE_DRIVER] >>>>>=20 >>>>=20 >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >> ReportStatusCodeLib|portStatusCodeLi >>>> b.inf >>>>>=20 >>>>> [LibraryClasses.common.UEFI_APPLICATION] >>>>>=20 >>>>=20 >> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeRe >> ReportStatusCodeLib|portStatusCodeLi >>>> b.inf >>>>>=20 >>>>> Thanks, >>>>> Star >>>>>=20 >>>>>>=20 >>>>>> Thanks, >>>>>>=20 >>>>>> Andrew Fish >>>>>>=20 >>>>>>> Thanks, >>>>>>> Star >>>>>>> -----Original Message----- >>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On=20 >>>>>>> Behalf Of Andrew >>>> Fish >>>>>>> Sent: Tuesday, August 9, 2016 7:08 AM >>>>>>> To: edk2-devel >>>>>>> Subject: [edk2] [MdeModulePkg] SetVirtualAddressMap() crashed=20 >>>>>>> due to >>>> DxeReportStatusCodeLib assuming the state of the BootService Memory at= runtime. >>>>>>>=20 >>>>>>> I was messing about with an ExitBootServices test that fills=20 >>>>>>> boot services >> memory >>>> with 0xAFAFAFAFAFAFAFAF (It was Vincent's idea to use my Initials=20 >>>> but it has the handy property of being a non-cononical address and=20 >>>> causes on GP fault on X64) and >>>> SetVirtualAddressMap() started crashing. >>>>>>>=20 >>>>>>> It looks like this code is assuming the 1st call to ReportStatus=20 >>>>>>> code will not >>>> happen at runtime. This is not the case for the RuntimeDxe driver. >>>>>>>=20 >>>>=20 >> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/Dx >> eReportStatusCod >>>> eLib/ReportStatusCodeLib.c#L43 >>>>>>> VOID >>>>>>> InternalGetReportStatusCode ( >>>>>>> VOID >>>>>>> ) >>>>>>> { >>>>>>> EFI_STATUS Status; >>>>>>>=20 >>>>>>> if (mReportStatusCodeLibStatusCodeProtocol !=3D NULL) { return; } >>>>>>>=20 >>>>>>> // >>>>>>> // Check gBS just in case ReportStatusCode is called before gBS is = initialized. >>>>>>> // >>>>>>> if (gBS !=3D NULL && gBS->LocateProtocol !=3D NULL) { Status =3D=20 >>>>>>> gBS->LocateProtocol (&gEfiStatusCodeRuntimeProtocolGuid, NULL, >> (VOID**) >>>> &mReportStatusCodeLibStatusCodeProtocol); >>>>>>> if (EFI_ERROR (Status)) { >>>>>>> mReportStatusCodeLibStatusCodeProtocol =3D NULL; } } } >>>>>>>=20 >>>>>>> I'm guessing this seems to work due >>>>=20 >> to:https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/Dx >> e/DxeMain/DxeMai >>>> n.c#L803 >>>>>>>=20 >>>>>>> // >>>>>>> // Zero out the Boot Service Table // ZeroMem (gBS, sizeof=20 >>>>>>> (EFI_BOOT_SERVICES)); >>>>>>>=20 >>>>>>>=20 >>>>>>> Thus if I'm looking at this code correctly it only looks like it=20 >>>>>>> works 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=20 >>>> of ExitBootServices, so it should be legal for my test to change the v= alue. >>>>>>>=20 >>>>>>> I wanted to get a few more eyes on this prior to filling a bug? >>>>>>>=20 >>>>>>> Thanks, >>>>>>>=20 >>>>>>> Andrew Fish >>>>>>>=20 >>>>>>>=20 >>>>>>> _______________________________________________ >>>>>>> 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 >>>>=20 >>>> _______________________________________________ >>>> edk2-devel mailing list >>>> edk2-devel@lists.01.org >>>> https://lists.01.org/mailman/listinfo/edk2-devel