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

[Shanker Donthineni <shankerd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

Hi Ard,

This patch causing the kernel boot fail using the UEFI firmware on
Qualcomm Technologies QDF2XXX server platforms.

+	/* RW- */
+	if (attr & EFI_MEMORY_XP || type != EFI_RUNTIME_SERVICES_CODE)
+		return pgprot_val(PAGE_KERNEL);
+

Changing above condition from 'or' to 'and' fixed the problem.Are we
missing something here or intentionally implemented this logic? At least
we need some pr_warn here if UEFI firmware passes EFI_RUNTIME_SEVRICE_CODE
region that has an attribute EFI_MEMORY_XP.



On 03/30/2016 11:38 AM, 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.
>
> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> ---
>  arch/arm64/kernel/efi.c | 54 +++++++++++++++-----
>  1 file changed, 40 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index b6abc852f2a1..33a6da160a50 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -17,22 +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.
> -	 */
> -	if ((md->attribute & EFI_MEMORY_WB) == 0)
> -		prot_val = PROT_DEVICE_nGnRE;
> -	else if (md->type == EFI_RUNTIME_SERVICES_CODE ||
> -		 !PAGE_ALIGNED(md->phys_addr))
> -		prot_val = pgprot_val(PAGE_KERNEL_EXEC);
> -	else
> -		prot_val = pgprot_val(PAGE_KERNEL);
> +	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.
> +		 */
> +		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,

-- 
Shanker Donthineni
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project