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?
Best regards
Heinrich
