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
