* [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
@ 2016-08-08 23:07 Andrew Fish
2016-08-09 1:21 ` Zeng, Star
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-08 23:07 UTC (permalink / raw)
To: edk2-devel
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/DxeReportStatusCodeLib/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/Dxe/DxeMain/DxeMain.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-08 23:07 [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime Andrew Fish
@ 2016-08-09 1:21 ` Zeng, Star
2016-08-09 2:07 ` Andrew Fish
0 siblings, 1 reply; 12+ messages in thread
From: Zeng, Star @ 2016-08-09 1:21 UTC (permalink / raw)
To: Andrew Fish, edk2-devel
Andrew,
Should MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib be used for your case if there are really runtime status code reporting needed?
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/DxeReportStatusCodeLib/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/Dxe/DxeMain/DxeMain.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-09 1:21 ` Zeng, Star
@ 2016-08-09 2:07 ` Andrew Fish
2016-08-09 2:26 ` Zeng, Star
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-09 2:07 UTC (permalink / raw)
To: Zeng, Star; +Cc: edk2-devel
> 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?
https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeReportStatusCodeLib/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/RuntimeDxe/RuntimeDxe.inf
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/DxeReportStatusCodeLib/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/Dxe/DxeMain/DxeMain.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-09 2:07 ` Andrew Fish
@ 2016-08-09 2:26 ` Zeng, Star
2016-08-09 14:50 ` Andrew Fish
0 siblings, 1 reply; 12+ messages in thread
From: Zeng, Star @ 2016-08-09 2:26 UTC (permalink / raw)
To: Andrew Fish; +Cc: edk2-devel
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.
>
> https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Library/DxeReportStatusCodeLib/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/RuntimeDxe/RuntimeDxe.inf
[LibraryClasses.common.DXE_CORE]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
[LibraryClasses.common.DXE_RUNTIME_DRIVER]
ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
[LibraryClasses.common.UEFI_DRIVER]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
[LibraryClasses.common.DXE_DRIVER]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
[LibraryClasses.common.UEFI_APPLICATION]
ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.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/DxeReportStatusCodeLib/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/Dxe/DxeMain/DxeMain.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-09 2:26 ` Zeng, Star
@ 2016-08-09 14:50 ` Andrew Fish
2016-08-10 17:22 ` Kinney, Michael D
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-09 14:50 UTC (permalink / raw)
To: Zeng, Star; +Cc: edk2-devel
> 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/DxeReportStatusCodeLib/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/RuntimeDxe/RuntimeDxe.inf
>
> [LibraryClasses.common.DXE_CORE]
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
>
> [LibraryClasses.common.UEFI_DRIVER]
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> [LibraryClasses.common.DXE_DRIVER]
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> [LibraryClasses.common.UEFI_APPLICATION]
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.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/DxeReportStatusCodeLib/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/Dxe/DxeMain/DxeMain.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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-09 14:50 ` Andrew Fish
@ 2016-08-10 17:22 ` Kinney, Michael D
2016-08-10 17:35 ` Andrew Fish
0 siblings, 1 reply; 12+ messages in thread
From: Kinney, Michael D @ 2016-08-10 17:22 UTC (permalink / raw)
To: Andrew Fish, Zeng, Star, Kinney, Michael D; +Cc: edk2-devel
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\RuntimeDxeReportStatusCodeLib.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
> -----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/DxeReportStatusCod
> 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/RuntimeDxe/RuntimeDxe
> .inf
> >
> > [LibraryClasses.common.DXE_CORE]
> >
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> b.inf
> >
> > [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> >
> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRepo
> rtStatusCodeLib.inf
> >
> > [LibraryClasses.common.UEFI_DRIVER]
> >
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> b.inf
> >
> > [LibraryClasses.common.DXE_DRIVER]
> >
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> b.inf
> >
> > [LibraryClasses.common.UEFI_APPLICATION]
> >
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> 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/DxeReportStatusCod
> 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/Dxe/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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-10 17:22 ` Kinney, Michael D
@ 2016-08-10 17:35 ` Andrew Fish
2016-08-10 18:02 ` Kinney, Michael D
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-10 17:35 UTC (permalink / raw)
To: Mike Kinney; +Cc: Zeng, Star, edk2-devel
> 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\RuntimeDxeReportStatusCodeLib.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?
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/DxeReportStatusCod
>> 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/RuntimeDxe/RuntimeDxe
>> .inf
>>>
>>> [LibraryClasses.common.DXE_CORE]
>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>> b.inf
>>>
>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRepo
>> rtStatusCodeLib.inf
>>>
>>> [LibraryClasses.common.UEFI_DRIVER]
>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>> b.inf
>>>
>>> [LibraryClasses.common.DXE_DRIVER]
>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>> b.inf
>>>
>>> [LibraryClasses.common.UEFI_APPLICATION]
>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>> 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/DxeReportStatusCod
>> 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/Dxe/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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-10 17:35 ` Andrew Fish
@ 2016-08-10 18:02 ` Kinney, Michael D
2016-08-10 19:04 ` Andrew Fish
0 siblings, 1 reply; 12+ messages in thread
From: Kinney, Michael D @ 2016-08-10 18:02 UTC (permalink / raw)
To: afish@apple.com, Kinney, Michael D; +Cc: Zeng, Star, edk2-devel
> -----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\RuntimeDxeReportStatusCodeLib.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.
>
> 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/DxeReportStatusCod
> >> 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/RuntimeDxe/RuntimeDxe
> >> .inf
> >>>
> >>> [LibraryClasses.common.DXE_CORE]
> >>>
> >>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> >> b.inf
> >>>
> >>> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
> >>>
> >>
> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRepo
> >> rtStatusCodeLib.inf
> >>>
> >>> [LibraryClasses.common.UEFI_DRIVER]
> >>>
> >>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> >> b.inf
> >>>
> >>> [LibraryClasses.common.DXE_DRIVER]
> >>>
> >>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> >> b.inf
> >>>
> >>> [LibraryClasses.common.UEFI_APPLICATION]
> >>>
> >>
> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
> >> 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/DxeReportStatusCod
> >> 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/Dxe/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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-10 18:02 ` Kinney, Michael D
@ 2016-08-10 19:04 ` Andrew Fish
2016-08-11 1:53 ` Zeng, Star
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-10 19:04 UTC (permalink / raw)
To: Mike Kinney; +Cc: Zeng, Star, edk2-devel
> 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\RuntimeDxeReportStatusCodeLib.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/DxeReportStatusCod
>>>> 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/RuntimeDxe/RuntimeDxe
>>>> .inf
>>>>>
>>>>> [LibraryClasses.common.DXE_CORE]
>>>>>
>>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>>>> b.inf
>>>>>
>>>>> [LibraryClasses.common.DXE_RUNTIME_DRIVER]
>>>>>
>>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeRepo
>>>> rtStatusCodeLib.inf
>>>>>
>>>>> [LibraryClasses.common.UEFI_DRIVER]
>>>>>
>>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>>>> b.inf
>>>>>
>>>>> [LibraryClasses.common.DXE_DRIVER]
>>>>>
>>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>>>> b.inf
>>>>>
>>>>> [LibraryClasses.common.UEFI_APPLICATION]
>>>>>
>>>>
>> ReportStatusCodeLib|MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLi
>>>> 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/DxeReportStatusCod
>>>> 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/Dxe/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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-10 19:04 ` Andrew Fish
@ 2016-08-11 1:53 ` Zeng, Star
2016-08-11 1:59 ` Andrew Fish
0 siblings, 1 reply; 12+ messages in thread
From: Zeng, Star @ 2016-08-11 1:53 UTC (permalink / raw)
To: afish@apple.com, Kinney, Michael D; +Cc: edk2-devel, Zeng, Star
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?
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-11 1:53 ` Zeng, Star
@ 2016-08-11 1:59 ` Andrew Fish
2016-08-11 3:06 ` Zeng, Star
0 siblings, 1 reply; 12+ messages in thread
From: Andrew Fish @ 2016-08-11 1:59 UTC (permalink / raw)
To: Zeng, Star; +Cc: Mike Kinney, edk2-devel
> 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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime.
2016-08-11 1:59 ` Andrew Fish
@ 2016-08-11 3:06 ` Zeng, Star
0 siblings, 0 replies; 12+ messages in thread
From: Zeng, Star @ 2016-08-11 3:06 UTC (permalink / raw)
To: Andrew Fish; +Cc: Mike Kinney, edk2-devel, star.zeng
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-08-11 3:07 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-08 23:07 [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime Andrew Fish
2016-08-09 1:21 ` Zeng, Star
2016-08-09 2:07 ` Andrew Fish
2016-08-09 2:26 ` Zeng, Star
2016-08-09 14:50 ` Andrew Fish
2016-08-10 17:22 ` Kinney, Michael D
2016-08-10 17:35 ` Andrew Fish
2016-08-10 18:02 ` Kinney, Michael D
2016-08-10 19:04 ` Andrew Fish
2016-08-11 1:53 ` Zeng, Star
2016-08-11 1:59 ` Andrew Fish
2016-08-11 3:06 ` Zeng, Star
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox