>-----Original Message----- >From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >Sent: Monday, June 29, 2015 8:35 AM >To: Yao, Jiewen >Cc: Fleming, Matt; edk2-devel@lists.sourceforge.net >Subject: Re: [edk2] BUG in properties table feature implementation > >On 29 June 2015 at 16:08, Yao, Jiewen <jiewen....@intel.com> wrote: >> Good to know. Thanks! >> > >Another question from my side: is it guaranteed the the memory map >returned by GetMemoryMap() is sorted? >Otherwise, it becomes non-trivial for the OS to ensure that all >Runtime regions are mapped adjacently. >
I have seen lots of odd maps with the memmap shell command. I think that the answer is no as a result, but I'd like to know if someone knows for sure. > >> -----Original Message----- >> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >> Sent: Monday, June 29, 2015 10:06 PM >> To: Yao, Jiewen >> Cc: edk2-devel@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; Justen, >Jordan L; Kinney, Michael D; Zeng, Star; Zimmer, Vincent; Fleming, Matt >> Subject: Re: BUG in properties table feature implementation >> >> On 29 June 2015 at 15:58, Yao, Jiewen <jiewen....@intel.com> wrote: >>> Yes. It seems we need some special handling for AArch64. >>> If 64KiB is minimal paging unit, I think we need check 64KiB PE section >alignment for AArch64. >>> >>> I confess that I only validated IA32 and X64, and I did not take AArch64 >>> into >account. >>> >>> BTW: There is no 64KiB alignment requirement for AArch32, right? I cannot >find the word. So I just want to double confirm. >>> >> >> No there is not. Only 64-bit ARM can execute with 64 KB page size. >> >> -- >> Ard. >> >> >>> -----Original Message----- >>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>> Sent: Monday, June 29, 2015 9:42 PM >>> To: Yao, Jiewen >>> Cc: edk2-devel@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; Justen, >Jordan L; Kinney, Michael D; Zeng, Star; Zimmer, Vincent; Fleming, Matt >>> Subject: Re: BUG in properties table feature implementation >>> >>> On 29 June 2015 at 14:54, Yao, Jiewen <jiewen....@intel.com> wrote: >>>> Thanks for the sharing. Yes. I agree with you that this breaks backward >compatibility. >>>> Right, I do not think we can boot Win7 or old Win8 with this feature >enabled. >>>> >>>> Both UEFI OS and Firmware need support this UEFI2.5 Properties Table >feature. I believe it is known by UEFI forum, when it is added to UEFI spec, >and it is published. >>>> >>> >>> Yes, but I don't think it is documented anywhere that the OS needs to map >all runtime regions adjacently even if it does not actually care about the >EFI_MEMORY_RO attributes. Essentially, the OS always need to map >adjacently to be compatible with UEFI 2.5, and the current AArch64 Linux >kernel crashes badly on any UEFI 2,5 implementation that has this feature >enabled. >>> >>>> UEFI 2.5 properties table is optional feature. It can be enabled or >disabled. >>>> The benefit of this feature is that: a UEFI2.5 OS can do memory protection >based on UEFI memory map. >>>> But the old UEFI OS cannot get any benefit. >>>> >>>> So a platform can made judgment based on its need, as well as the UEFI >2.5 OS availability. >>>> >>> >>> Unfortunately, this makes it an opt-in setting, and system will ship with it >disabled by default, in order to prevent boot failures on older OSes. That is >not typically how you want to deploy a security feature. >>> >>> There is another issue I would like your opinion about: >>> for AArch64, MdeModulePkg/Core/Dxe/Mem/Imem.h defines the >following >>> >>> #define EFI_ACPI_RUNTIME_PAGE_ALLOCATION_ALIGNMENT >(SIZE_64KB) >>> >>> and in CoreTerminateMemoryMap(), a final check is done that no Runtime >regions are aligned incorrectly. >>> Yet, with the Properties Table feature enabled, memory regions are split >without taking this into account. Strangely enough, >CoreTerminateMemoryMap () does not complain about this. >>> >>> This is documented in the UEFI spec 2.3.6: >>> """ >>> All 4KiB memory pages allocated for use by runtime services (of types >EfiRuntimeServicesCode, EfiRuntimeServicesData and >>> EfiACPIMemoryNVS) must use identical ARM Memory Page Attributes (as >described in Table 8) within the same physical 64KiB page. Mixed attribute >mappings within a larger page are not allowed. >>> """ >>> >>> So I suppose the Properties table feature should use 64 KB not 4 KB when >executing on AArch64. However, the documentation for the Properties Table >feature mentions 4 KB explicitly, so that should go into the errata as well. >>> >>> >>> Regards, >>> Ard. >>> >>> >>>> That is why we do not enable this PE/COFF 4K alignment in BaseTool as >standard configuration. >>>> So in other word, it is disabled by tool by default. A platform may also >decide to not publish PropertiesTable by below PCD: >>>> ## Publish PropertiesTable or not. >>>> # >>>> # If this PCD is TRUE, DxeCore publishs PropertiesTable. >>>> # DxeCore evaluates if all runtime drivers has 4K aligned PE sections. >>>> If all >>>> # PE sections in runtime drivers are 4K aligned, DxeCore sets BIT0 in >>>> # PropertiesTable. Or DxeCore clears BIT0 in PropertiesTable. >>>> # If this PCD is FALSE, DxeCore does not publish PropertiesTable. >>>> # >>>> # If PropertiesTable has BIT0 set, DxeCore uses below policy in UEFI >memory map: >>>> # 1) Use EfiRuntimeServicesCode for runtime driver PE image code >section and >>>> # use EfiRuntimeServicesData for runtime driver PE image header and >other section. >>>> # 2) Set EfiRuntimeServicesCode to be EFI_MEMORY_RO. >>>> # 3) Set EfiRuntimeServicesData to be EFI_MEMORY_XP. >>>> # 4) Set EfiMemoryMappedIO and EfiMemoryMappedIOPortSpace to >be EFI_MEMORY_XP. >>>> # >>>> # NOTE: Platform need gurantee this PCD is set correctly. Platform >should set >>>> # this PCD to be TURE if and only if all runtime driver has seperated >Code/Data >>>> # section. If PE code/data sections are merged, the result is >unpredictable. >>>> # >>>> # @Prompt Publish UEFI PropertiesTable. >>>> >>>> >gEfiMdeModulePkgTokenSpaceGuid.PropertiesTableEnable|TRUE|BOOLEAN >|0x00 >>>> 00006e >>>> >>>> Thank you >>>> Yao Jiewen >>>> >>>> >>>> -----Original Message----- >>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>>> Sent: Monday, June 29, 2015 8:44 PM >>>> To: Yao, Jiewen >>>> Cc: edk2-devel@lists.sourceforge.net; Gao, Liming; Laszlo Ersek; >>>> Justen, Jordan L; Kinney, Michael D; Zeng, Star >>>> Subject: Re: BUG in properties table feature implementation >>>> >>>> On 29 June 2015 at 14:34, Yao, Jiewen <jiewen....@intel.com> wrote: >>>>> Hi Ard >>>>> Yes, your are right. We do observe similar odd behavior in old >Win7/Win8/Win8.1, and we have to disable it. >>>>> Win8 and Win8.1 have OS patch to set adjacent virtual memory map, to >resolve this issue. >>>> >>>> I see this as a big problem with the feature, as it breaks backward >>>> compatibility. IOW, you won't be able to boot Windows 7 on a UEFI 2.5 >>>> system unless you manually disable the feature (in the setup screen, I >>>> suppose?) >>>> >>>>> We also tested Win10, and Suse 11 GM. They are good. >>>>> >>>>> Would you please share the information on which OS you are using? >>>>> >>>> >>>> I am using the upstream Linux kernel for AArch64. I am not using a >distribution. >>>> >>>>> All in all, if OS cannot guarantee the virtual memory map is adjacent, we >have to disable this feature. >>>>> >>>> >>>> This is backwards: the feature should be implemented such that it cannot >be trivially broken by a well-behaving OS. Note that nothing in the >SetVirtualAddressMap () implementation suggests that the regions should be >adjacent. >>>> >>>> Perhaps it would have been better to introduce a new memory region >>>> type EfiRuntimeServicesPecoffData, so the OS at least knows which >>>> regions it should keep adjacent, (i.e., Code and PecoffData regions >>>> should be kept together) >>>> >>>> So does PE/COFF even allow the memory image to deviate from the file >image? If so, the correct way is to update the PE/COFF loader and the >relocation logic can deal with sections being laid out differently in memory >than they are in the file. >>>> >>>> -- >>>> Ard. >>>> >>>>> -----Original Message----- >>>>> From: Ard Biesheuvel [mailto:ard.biesheu...@linaro.org] >>>>> Sent: Monday, June 29, 2015 6:46 PM >>>>> To: edk2-devel@lists.sourceforge.net; Yao, Jiewen; Gao, Liming; >>>>> Laszlo Ersek; Justen, Jordan L; Kinney, Michael D; Zeng, Star >>>>> Subject: BUG in properties table feature implementation >>>>> >>>>> Hello all, >>>>> >>>>> I am running into another problem with the implementation of the UEFI >>>>> 2.5 Properties Table feature. It splits PE/COFF images into separate but >adjacent memory regions, only to be able to assign different permissions to >.text and .data sections. This is working fine at boot time. >>>>> >>>>> However, at runtime, after calling virtual address map, this breaks >>>>> down completely. Since the virtual mapping supplied to >>>>> SetVirtualAddressMap() does not have to guarantee adjacency >between code and data regions (of which the OS does not know whether >they belong together or not), reapplying the relocations corrupts the memory >image and breaks the runtime services. >>>>> >>>>> For example, this region >>>>> >>>>> 0x00005eeb1000-0x00005eeb6fff [Runtime Code] >>>>> 0x00005eeb7000-0x00005eec0fff [Runtime Data] >>>>> >>>>> is mapped on AARCH64 as >>>>> >>>>> EFI remap 0x000000005eeb1000 => 00000000440a1000 >>>>> EFI remap 0x000000005eeb7000 => 00000000440b7000 >>>>> >>>>> which retains the relative alignment, but adds a 64 KB offset to the >second regions so that the regions can still be mapped with different >permissions when the OS is executing with a 64 KB page size. >>>>> >>>>> As far as I can tell, this is in accordance with the spec, and was working >fine until I tried to enable the properties table feature. >>>>> With that enabled, the two above regions could actually describe one >single PE/COFF image in memory, and the 64 KB offset results in the >relocations to be applied incorrectly. >>>>> >>>>> I looked at PeCoffLoaderRelocateImageForRuntime () but to me, it is >>>>> not very obvious how to solve this. Obviously, our PE/COFF >>>>> implementation is not complete since it assumes file offset == memory >>>>> offset for sections, but this does not hold anymore for UEFI 2.5 >>>>> >>>>> I would also like to point out again that this is another result of the >>>>> fact >that this series was pushed through with any review or testing outside of the >Intel firmware team. For features of this magnitude and complexity, more >scrutiny and testing is obviously required. >>>>> >>>>> Kind regards, >>>>> Ard. > >------------------------------------------------------------------------------ >Monitor 25 network devices or servers for free with OpManager! >OpManager is web-based network management software that monitors >network devices and physical & virtual servers, alerts via email & sms >for fault. Monitor 25 devices for free with no restriction. Download now >http://ad.doubleclick.net/ddm/clk/292181274;119417398;o >_______________________________________________ >edk2-devel mailing list >edk2-devel@lists.sourceforge.net >https://lists.sourceforge.net/lists/listinfo/edk2-devel ------------------------------------------------------------------------------ Monitor 25 network devices or servers for free with OpManager! OpManager is web-based network management software that monitors network devices and physical & virtual servers, alerts via email & sms for fault. Monitor 25 devices for free with no restriction. Download now http://ad.doubleclick.net/ddm/clk/292181274;119417398;o _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
