From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-in4.apple.com (mail-out4.apple.com [17.151.62.26]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 4BD4A1A1DED for ; Wed, 10 Aug 2016 18:59:35 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; d=apple.com; s=mailout2048s; c=relaxed/simple; q=dns/txt; i=@apple.com; t=1470880774; x=2334794374; h=From:Sender:Reply-To:Subject:Date:Message-id:To:Cc:MIME-version:Content-type: Content-transfer-encoding:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:In-reply-to:References:List-Id: List-Help:List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=tHQJZbwBGePemXHuO6iKZpuUxrYYXRAK/Ik92fRReEY=; b=HRToccmc7o7LRzGljjn1qyFYyTZ1EJBU+8Sgn4vQZd1JFMj0VN7lYoQq020SMQ55 R53D/cTNICrcB9asRGzHldTxVNLzeRZiA09SqTgO8HvGhCaMVs1o+zEmNr7CLZGr EEYgR7qRuxevBNMQcsJj443udNWhil6NE0P1HPyZcrjO+iyqshKs7Igg1EFwFFBb N1u+HuF7GBc2b96t6WE56u7Q1CcURe6SGThD/b3tH7udtLUrMYQxx57V3rRYz23r i3gbSX5bhhO476HoOSt6BvS9+zFxbqZXl2jzURYNgQbznY8JFAIJ3w0bSKf6IZu4 +Z1NjoWK8rJxYBBBDZS/5A==; Received: from relay2.apple.com (relay2.apple.com [17.128.113.67]) by mail-in4.apple.com (Apple Secure Mail Relay) with SMTP id A9.D6.07433.60CDBA75; Wed, 10 Aug 2016 18:59:34 -0700 (PDT) X-AuditID: 11973e12-f79b16d000001d09-16-57abdc06b93d Received: from nwk-mmpp-sz12.apple.com (nwk-mmpp-sz12.apple.com [17.128.115.204]) by relay2.apple.com (Apple SCV relay) with SMTP id B4.10.01452.60CDBA75; Wed, 10 Aug 2016 18:59:34 -0700 (PDT) MIME-version: 1.0 Received: from [17.153.29.18] by nwk-mmpp-sz12.apple.com (Oracle Communications Messaging Server 8.0.1.1.0 64bit (built Jun 15 2016)) with ESMTPSA id <0OBQ00J252V98K90@nwk-mmpp-sz12.apple.com>; Wed, 10 Aug 2016 18:59:34 -0700 (PDT) Sender: afish@apple.com From: Andrew Fish In-reply-to: <0C09AFA07DD0434D9E2A0C6AEB048310036A2959@shsmsx102.ccr.corp.intel.com> Date: Wed, 10 Aug 2016 18:59:33 -0700 Cc: Mike Kinney , edk2-devel Message-id: <4F6AB174-5C49-4E59-98F9-6E4B57DA31BF@apple.com> References: <776695E1-E864-42F4-A633-B86FB913A510@apple.com> <0C09AFA07DD0434D9E2A0C6AEB048310036A1BCF@shsmsx102.ccr.corp.intel.com> <33692B19-74EE-4C13-8AE3-1C92B74836C3@apple.com> <9e979fb5-52ac-efe4-480e-8670d8233a7c@intel.com> <50FCE63D-C671-481E-B321-B0D06356A871@apple.com> <0C09AFA07DD0434D9E2A0C6AEB048310036A2959@shsmsx102.ccr.corp.intel.com> To: "Zeng, Star" X-Mailer: Apple Mail (2.3112) X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrLLMWRmVeSWpSXmKPExsUi2FDorMt2Z3W4QWOLhcWeQ0eZLTo6/jFZ 7Ou1dmD2WLznJZNH9+x/LAFMUVw2Kak5mWWpRfp2CVwZW479ZinYUFLRvPkTawPj/IQuRk4O CQETiYn9y9khbDGJC/fWs3UxcnEICexllDh+5ykzTNH9S39ZIRKHGCW+z/7HCJLgFRCU+DH5 HksXIwcHs4C8xMHzsiBhZgEtie+PWlkg6t8ySiyc18cCkhAWEJd4d2YTM0hCWGAWo8SLBTPA EmwCyhIr5n8AO4NTIExiyd0VrCA2i4CqxIQfd1khpoZIHL4xjQVisY3EwYn7mCE2PGaReDH1 IBtIQkRATWLv6l1Q/8hK7NuwAOwfCYE9bBIbX69hmsAoMgvJ5bMQLp+F5PIFjMyrGIVyEzNz dDPzTPQSCwpyUvWS83M3MYLCf7qd0A7GU6usDjEKcDAq8fB6ZK4OF2JNLCuuzD3EKM3BoiTO O38qUEggPbEkNTs1tSC1KL6oNCe1+BAjEwenVAPjpGUPn7qnmTunpUlEeq1yue6TqRp+mK9s x+JdC8pqOWLWc26ZUBNt/03m8MG2vhKRjhdJ9ov3vnzWo/Z01h6L525rHzI/P3Igy3zJk2/b PA7oNCtvU+w4vFX0wjGhxhWvOdv3TtSxKmNrLitp9n6TU+BgVeQd8OJyzZGF/oUbuJ65M5u4 r+BSYinOSDTUYi4qTgQATcL8fWACAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFupgkeLIzCtJLcpLzFFi42IRbCg+o8t2Z3W4wYmLxhZ7Dh1ltujo+Mdk sa/X2oHZY/Gel0we3bP/sQQwRXHZpKTmZJalFunbJXBlbDn2m6VgQ0lF8+ZPrA2M8xO6GDk5 JARMJO5f+ssKYYtJXLi3nq2LkYtDSOAQo8T32f8YQRK8AoISPybfY+li5OBgFpCXOHheFiTM LKAl8f1RKwtE/VtGiYXz+lhAEsIC4hLvzmxiBkkIC8xilHixYAZYgk1AWWLF/A/sIDanQJjE krsrwDazCKhKTPhxlxViaojE4RvTWCAW20gcnLiPGWLDYxaJF1MPsoEkRATUJPau3sUOcbas xL4NC9gmMArOQnLsLIRjZyE5dgEj8ypGgaLUnMRKI73EgoKcVL3k/NxNjOBwLXTewXhsmdUh RgEORiUeXo/M1eFCrIllxZW5wNDgYFYS4RW6DBTiTUmsrEotyo8vKs1JLT7EmAz0wERmKdHk fGAs5ZXEG5qYGJgYG5sZG5ubmJMmrCTO67IIaIVAemJJanZqakFqEcwWJg5OqQbGtjsNCjdv LTj9dfbf7kdJMYrPt52/NynbvJHjbOzl8JRb/GteZ09I+OWg5nxT3ilaenOyqc+aiB1h1T6z W313vNZ94Tu/NmJKzPvZEQvMJq7oV5ksPNfyQ/S3sE0G2y7HiKkmRk27q2367x3D78drLQ9+ 3WC8qq/ba+FPwcWb05Z0vfK9HvNngRJLcUaioRZzUXEiANk0uD6bAgAA Subject: Re: [MdeModulePkg] SetVirtualAddressMap() crashed due to DxeReportStatusCodeLib assuming the state of the BootService Memory at runtime. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Aug 2016 01:59:35 -0000 Content-transfer-encoding: 7BIT Content-type: text/plain; CHARSET=US-ASCII > On Aug 10, 2016, at 6:53 PM, Zeng, Star 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 > Cc: Zeng, Star ; edk2-devel > 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 wrote: >> >> >> >>> -----Original Message----- >>> From: afish@apple.com [mailto:afish@apple.com] >>> Sent: Wednesday, August 10, 2016 10:35 AM >>> To: Kinney, Michael D >>> Cc: Zeng, Star ; edk2-devel >>> >>> 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 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 >>>>> Cc: edk2-devel >>>>> 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 wrote: >>>>>> >>>>>> On 2016/8/9 10:07, Andrew Fish wrote: >>>>>>> >>>>>>>> On Aug 8, 2016, at 6:21 PM, Zeng, Star 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 >>>>>>>> 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