Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.

[Andrew Fish <afish@apple.com>]

> On Aug 10, 2016, at 6:53 PM, Zeng, Star <star.zeng@intel.com> 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: afish@apple.com [mailto:afish@apple.com] 
> Sent: Thursday, August 11, 2016 3:04 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel <edk2-devel@lists.01.org>
> 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 <michael.d.kinney@intel.com> wrote:
>> 
>> 
>> 
>>> -----Original Message-----
>>> From: afish@apple.com [mailto:afish@apple.com]
>>> Sent: Wednesday, August 10, 2016 10:35 AM
>>> To: Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Zeng, Star <star.zeng@intel.com>; edk2-devel 
>>> <edk2-devel@lists.01.org>
>>> 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 <michael.d.kinney@intel.com> 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 <star.zeng@intel.com>
>>>>> Cc: edk2-devel <edk2-devel@lists.01.org>
>>>>> 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 <star.zeng@intel.com> wrote:
>>>>>> 
>>>>>> On 2016/8/9 10:07, Andrew Fish wrote:
>>>>>>> 
>>>>>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star <star.zeng@intel.com> 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 <edk2-devel@lists.01.org>
>>>>>>>> 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