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

["Zeng, Star" <star.zeng@intel.com>]

On 2016/8/11 9:59, Andrew Fish wrote:
>
>> 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.

Andrew,

I see the point, and I also agree it is helpful with good rules.
I just thought about a possible valid use case from user, and propose to 
reduce the negative impact.
Adding notes like DxePcdLib.inf is a possible approach to state the 
limitation, and there are also similar notes in SecPeiDxeTimerLibCpu.inf 
and SecPeiDxeTimerLibUefiCpu.inf.


Thanks,
Star

>
> 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