[PATCH] arm64: efi: Make runtime region misalignment warning less noisy

[Heinrich Schuchardt]

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 at canonical.com>
>> Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
>> Reported-by: Linus Torvalds <torvalds at linux-foundation.org>
>> Signed-off-by: Ard Biesheuvel <ardb at kernel.org>
>> ---
>>   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.

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 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().

Best regards

Heinrich

> 
> The warning tells that Linux cannot establish secure settings for some 
> pages. It would be preferable to keep it and fix the UEFI specification 
> and the firmware instead.
> 
> Best regards
> 
> Heinrich
> 
>>           /*
>>            * If the region is not aligned to the page size of the OS, we
>>            * can not use strict permissions, since that would also affect