On 2 March 2016 at 13:10, Matt Fleming <matt@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>> ---
>> 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,
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
