> On Aug 10, 2016, at 6:53 PM, Zeng, Star <[email protected]> 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. 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: [email protected] [mailto:[email protected]] > Sent: Thursday, August 11, 2016 3:04 AM > To: Kinney, Michael D <[email protected]> > Cc: Zeng, Star <[email protected]>; edk2-devel <[email protected]> > 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 <[email protected]> >> wrote: >> >> >> >>> -----Original Message----- >>> From: [email protected] [mailto:[email protected]] >>> Sent: Wednesday, August 10, 2016 10:35 AM >>> To: Kinney, Michael D <[email protected]> >>> Cc: Zeng, Star <[email protected]>; edk2-devel >>> <[email protected]> >>> 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 >>>> <[email protected]> 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:[email protected]] On Behalf >>>>> Of Andrew Fish >>>>> Sent: Tuesday, August 9, 2016 7:50 AM >>>>> To: Zeng, Star <[email protected]> >>>>> Cc: edk2-devel <[email protected]> >>>>> 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 <[email protected]> wrote: >>>>>> >>>>>> On 2016/8/9 10:07, Andrew Fish wrote: >>>>>>> >>>>>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star <[email protected]> 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:[email protected]] On >>>>>>>> Behalf Of Andrew >>>>> Fish >>>>>>>> Sent: Tuesday, August 9, 2016 7:08 AM >>>>>>>> To: edk2-devel <[email protected]> >>>>>>>> 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 >>>>>>>> [email protected] >>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>>>> _______________________________________________ >>>>>>>> edk2-devel mailing list >>>>>>>> [email protected] >>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>> >>>>>> _______________________________________________ >>>>>> edk2-devel mailing list >>>>>> [email protected] >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> [email protected] >>>>> https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
