[PATCH 2/5] arm64: efi: apply strict permissons for UEFI Runtime Services regions

[Ard Biesheuvel]

On 2 March 2016 at 13:10, Matt Fleming <matt at codeblueprint.co.uk> wrote:
> On Mon, 22 Feb, at 03:25:55PM, Ard Biesheuvel wrote:
>> Recent UEFI versions expose permission attributes for runtime services
>> memory regions, either in the UEFI memory map or in the separate memory
>> attributes table. This allows the kernel to map these regions with
>> stricter permissions, rather than the RWX permissions that are used by
>> default. So wire this up in our mapping routine.
>>
>> Note that in the absence of permission attributes, we still only map
>> regions of type EFI_RUNTIME_SERVICE_CODE with the executable bit set.
>> Also, we base the mapping attributes of EFI_MEMORY_MAPPED_IO on the
>> type directly rather than on the absence of the EFI_MEMORY_WB attribute.
>> This is more correct, but is also required for compatibility with the
>> upcoming support for the Memory Attributes Table, which only carries
>> permission attributes, not memory type attributes.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel at linaro.org>
>> ---
>>  arch/arm64/kernel/efi.c | 27 ++++++++++++++++----
>>  1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
>> index b6abc852f2a1..3364408c154f 100644
>> --- a/arch/arm64/kernel/efi.c
>> +++ b/arch/arm64/kernel/efi.c
>> @@ -24,15 +24,32 @@ int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
>>       /*
>>        * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
>>        * executable, everything else can be mapped with the XN bits
>> -      * set.
>> +      * set. Also take the new (optional) RO/XP bits into account.
>>        */
>> -     if ((md->attribute & EFI_MEMORY_WB) == 0)
>> +     if (md->type == EFI_MEMORY_MAPPED_IO)
>>               prot_val = PROT_DEVICE_nGnRE;
>> -     else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
>> -              !PAGE_ALIGNED(md->phys_addr))
>> +     else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
>> +                        "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
>> +             /*
>> +              * If the region is not aligned to the page size of the OS, we
>> +              * can not use strict permissions, since that would also affect
>> +              * the mapping attributes of the adjacent regions.
>> +              */
>>               prot_val = pgprot_val(PAGE_KERNEL_EXEC);
>> -     else
>> +     else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
>> +              (EFI_MEMORY_XP | EFI_MEMORY_RO))
>> +             /* R-- */
>> +             prot_val = pgprot_val(PAGE_KERNEL_RO);
>> +     else if (md->attribute & EFI_MEMORY_RO)
>> +             /* R-X */
>> +             prot_val = pgprot_val(PAGE_KERNEL_ROX);
>> +     else if (md->attribute & EFI_MEMORY_XP ||
>> +              md->type != EFI_RUNTIME_SERVICES_CODE)
>> +             /* RW- */
>>               prot_val = pgprot_val(PAGE_KERNEL);
>> +     else
>> +             /* RWX */
>> +             prot_val = pgprot_val(PAGE_KERNEL_EXEC);
>>
>>       create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
>>                          md->num_pages << EFI_PAGE_SHIFT,
>
> The actual logic looks fine but it seems like there's quite a lot
> going on in this function which is fairly difficult to decipher with
> the if/else if clauses.
>
> Would you be open to splitting this out a little? It's just a
> suggestion, but maybe something like this,
>

Sure, that looks a lot better. I will fold that into v2

> ---
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index 3364408c154f..33a6da160a50 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -17,39 +17,48 @@
>
>  #include <asm/efi.h>
>
> -int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> +/*
> + * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
> + * executable, everything else can be mapped with the XN bits
> + * set. Also take the new (optional) RO/XP bits into account.
> + */
> +static __init pteval_t create_mapping_protection(efi_memory_desc_t *md)
>  {
> -       pteval_t prot_val;
> +       u64 attr = md->attribute;
> +       u32 type = md->type;
>
> -       /*
> -        * Only regions of type EFI_RUNTIME_SERVICES_CODE need to be
> -        * executable, everything else can be mapped with the XN bits
> -        * set. Also take the new (optional) RO/XP bits into account.
> -        */
> -       if (md->type == EFI_MEMORY_MAPPED_IO)
> -               prot_val = PROT_DEVICE_nGnRE;
> -       else if (WARN_ONCE(!PAGE_ALIGNED(md->phys_addr),
> -                          "UEFI Runtime regions are not aligned to 64 KB -- buggy firmware?"))
> +       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 the region is not aligned to the page size of the OS, we
>                  * can not use strict permissions, since that would also affect
>                  * the mapping attributes of the adjacent regions.
>                  */
> -               prot_val = pgprot_val(PAGE_KERNEL_EXEC);
> -       else if ((md->attribute & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
> -                (EFI_MEMORY_XP | EFI_MEMORY_RO))
> -               /* R-- */
> -               prot_val = pgprot_val(PAGE_KERNEL_RO);
> -       else if (md->attribute & EFI_MEMORY_RO)
> -               /* R-X */
> -               prot_val = pgprot_val(PAGE_KERNEL_ROX);
> -       else if (md->attribute & EFI_MEMORY_XP ||
> -                md->type != EFI_RUNTIME_SERVICES_CODE)
> -               /* RW- */
> -               prot_val = pgprot_val(PAGE_KERNEL);
> -       else
> -               /* RWX */
> -               prot_val = pgprot_val(PAGE_KERNEL_EXEC);
> +               return pgprot_val(PAGE_KERNEL_EXEC);
> +
> +       /* R-- */
> +       if ((attr & (EFI_MEMORY_XP | EFI_MEMORY_RO)) ==
> +           (EFI_MEMORY_XP | EFI_MEMORY_RO))
> +               return pgprot_val(PAGE_KERNEL_RO);
> +
> +       /* R-X */
> +       if (attr & EFI_MEMORY_RO)
> +               return pgprot_val(PAGE_KERNEL_ROX);
> +
> +       /* RW- */
> +       if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
> +               return pgprot_val(PAGE_KERNEL);
> +
> +       /* RWX */
> +       return pgprot_val(PAGE_KERNEL_EXEC);
> +}
> +
> +int __init efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md)
> +{
> +       pteval_t prot_val = create_mapping_protection(md);
>
>         create_pgd_mapping(mm, md->phys_addr, md->virt_addr,
>                            md->num_pages << EFI_PAGE_SHIFT,