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

[Heinrich Schuchardt]

On 11/6/22 10:48, Ard Biesheuvel wrote:
> On Sun, 6 Nov 2022 at 03:27, Heinrich Schuchardt
> <heinrich.schuchardt at canonical.com> 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 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.
>>
> 
> 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