On Sun, 6 Nov 2022 at 11:44, Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx> wrote: > > > > On 11/6/22 10:48, Ard Biesheuvel wrote: > > On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt > > <heinrich.schuchardt@xxxxxxxxxxxxx> wrote: > >> > >> > >> > >> On 11/6/22 00:24, Heinrich Schuchardt wrote: > >>> On 11/5/22 23:52, Ard Biesheuvel wrote: > >>>> The EFI spec requires that on arm64 systems, all runtime code and data > >>>> regions that share a 64k page can be mapped with the same memory type > >>>> attributes. Unfortunately, this does not take permission attributes into > >>>> account, and so the firmware is permitted to expose runtime code and > >>>> data regions that share 64k pages, and this may prevent the OS from > >>>> using restricted permissions in such cases, e.g., map data regions with > >>>> non-exec attributes. > >>> > >>> This is the relevant paragraph in the UEFI specification: > >>> > >>> <cite> > >>> The ARM architecture allows mapping pages at a variety of granularities, > >>> including 4KiB and 64KiB. If a 64KiB physical page contains any 4KiB > >>> page with any of the following types listed below, then all 4KiB pages > >>> in the 64KiB page must use identical ARM Memory Page Attributes (as > >>> described in Map EFI Cacheability Attributes to AArch64 Memory Types): > >>> > >>> - EfiRuntimeServicesCode > >>> - EfiRuntimeServicesData > >>> - EfiReserved > >>> - EfiACPIMemoryNVS > >>> > >>> Mixed attribute mappings within a larger page are not allowed. > >>> </cite> > >>> > >>> It remains unclear if only EFI Cacheability of also other page > >>> attributes are meant. The UEFI specification should be clarified in this > >>> respect. > >>> > >>>> > >>>> We currently emit a warning when hitting this at boot, but the warning > >>>> is problematic for a number of reasons: > >>>> - it uses WARN() which spews a lot of irrelevant information into the > >>>> log about the execution context where the issue was detected; > >>>> - it only takes the start of the region into account and not the size > >>> > >>> Is the occurrence of the warning specific to U-Boot or do you see the > >>> warning with EDK II too? > >>> > >>>> > >>>> Let's just drop the warning, as the condition does not strictly violate > >>>> the spec (although it only occurs with U-Boot), and fix the check to > >>>> take both the start and the end addresses into account. > >>>> > >>>> Cc: Heinrich Schuchardt <heinrich.schuchardt@xxxxxxxxxxxxx> > >>>> Cc: Ilias Apalodimas <ilias.apalodimas@xxxxxxxxxx> > >>>> Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> > >>>> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx> > >>>> --- > >>>> arch/arm64/kernel/efi.c | 4 ++-- > >>>> 1 file changed, 2 insertions(+), 2 deletions(-) > >>>> > >>>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > >>>> index e1be6c429810d0d5..3dd6f0c66f8aeb78 100644 > >>>> --- a/arch/arm64/kernel/efi.c > >>>> +++ b/arch/arm64/kernel/efi.c > >>>> @@ -25,8 +25,8 @@ static __init pteval_t > >>>> create_mapping_protection(efi_memory_desc_t *md) > >>>> if (type == EFI_MEMORY_MAPPED_IO) > >>>> return PROT_DEVICE_nGnRE; > >>>> - if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr), > >>>> - "UEFI Runtime regions are not aligned to 64 KB -- buggy > >>>> firmware?")) > >>>> + if (!PAGE_ALIGNED(md->phys_addr) || > >>>> + !PAGE_ALIGNED(md->num_pages * EFI_PAGE_SIZE)) > >>> > >>> Enhancing the check is correct. > >> > >> The UEFI requirement is that within a 64 KiB page all memory descriptors > >> shall use the same page attributes if any 4 KiB sub-page is of one of > >> the following types. > >> > >> - EfiRuntimeServicesCode > >> - EfiRuntimeServicesData > >> - EfiReserved > >> - EfiACPIMemoryNVS > >> > >> It is not required that memory descriptors shall be aligned to 64 KiB > >> boundaries. > >> > > > > Indeed, this is what I misremembered. > > > >> So the following map should not pose any problem: > >> > >> 00000-00fff - EfiBootServicesData (not used at runtime) > >> 01000-13fff - EfiRuntimeServicesData > >> 14000-1ffff - EfiRuntimeServicesData > >> 20000-24fff - EfiRuntimeServicesCode > >> 25000-27fff - EfiBootServicesCode (not used at runtime) > >> 28000-3ffff - EfiRuntimeServicesCode > >> > >> Evaluating each memory descriptor individually looks wrong. You first > >> have to extend each memory descriptor of one of the four aforementioned > >> memory types to the next 64 KiB boundary or within a 64 KiB boundary to > >> the next descriptor of one of the aforementioned memory types. Next you > >> have to merge adjacent descriptors with same attributes within the same > >> 64 KiB page. > >> > > > > So now we have to look at adjacent descriptors, which means we have to > > sort the memory map, as there is no guarantee that the descriptors > > appear in order. > > > >> So the map for which you set attributes would become > >> > >> 00000-1ffff - EfiRuntimeServicesData > >> 20000-3ffff - EfiRuntimeServicesCode > >> > >> I guess all that alignment and merging should go into efi_virtmap_init(). > >> > > > > U-boot does not provide a memory attributes table either, so we don't > > know which parts of the code regions should be mapped R-X and which > > parts RW- (Firmware implementations such as EDK2 that are based on > > PE/COFF images internally use code descriptors for each executable, > > which means they cover both the .text/.rodata and .data/.bss sections > > of the image. The data descriptors are used for dynamic allocations). > > > > This is why we use RWX for RTcode and RW- for RTdata in absence of the > > RO/XP attributes (which are passed via the memory attributes table > > usually). > > > > So in summary, I think the patch is fine. The warning is spurious > > given that the condition in question is actually permitted by the > > spec. > > > > On the uboot side, which already seems to align and round up RTcode > > sections to 64k, we might set the EFI_MEMORY_RO attribute on such > > regions if they really only contain .text and .rodata segments, and > > can tolerate being mapped without writable permissions. That way, the > > kernel will understand that it does not need to provide RWX > > permissions, which is really what all this code is trying to prevent. > > Shouldn't EFI_MEMORY_RO only be set if the UEFI firmware actually sets > up the MMU to make the corresponding memory read only? > No. The EFI_MEMORY_RO and XP attributes describe the nature of the contents of the regions, i.e., if they support being mapped with read-only resp. non-executable permissions. The same applies to the memory type attributes, btw: on bare metal, the memory is usually described as WC|WT|WB and it is up to the OS to choose between memory types when it creates the mapping - how the firmware maps it is irrelevant. In general, the OS does not care or even tries to determine how the firmware has programmed the MMU and the page tables.
