[U-Boot] [PATCH v2] efi_loader: Make RTS relocation more robust

[Heinrich Schuchardt]

On 12/11/18 10:00 AM, Alexander Graf wrote:
> While changing the RTS alignment to 64KB in commit 7a82c3051c8f
> ("efi_loader: Align runtime section to 64kb") the relocation code
> started to break.
> 
> The reason for that is that we didn't actually look at the real
> relocation data. We merely took the RUNTIME_CODE section as a
> hint and started to relocate based on self calculated data from
> that point on. That calculation was now out of sync though.
> 
> To ensure we're not running into such a situation again, this patch
> makes the runtime relocation code a bit more robust. We can just
> trust the phys/virt hints from the payload. We also should check that
> we really only have a single section, as the code doesn't handle
> multiple code relocations yet.
> 
> Fixes: 7a82c3051c8f ("efi_loader: Align runtime section to 64kb")
> Reported-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Reported-by: Loic Devulder <ldevulder at suse.de>
> Signed-off-by: Alexander Graf <agraf at suse.de>

Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

> 
> ---
> 
> v1 -> v2:
> 
>   - Add more verbose comment explaining why we have the
>     sanity check.
> ---
>  lib/efi_loader/efi_runtime.c | 34 +++++++++++++++++++++++++++++++---
>  1 file changed, 31 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index 95844efdb0..fff93f0960 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -436,14 +436,42 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  			uint32_t descriptor_version,
>  			struct efi_mem_desc *virtmap)
>  {
> -	ulong runtime_start = (ulong)&__efi_runtime_start &
> -			      ~(ulong)EFI_PAGE_MASK;
>  	int n = memory_map_size / descriptor_size;
>  	int i;
> +	int rt_code_sections = 0;
>  
>  	EFI_ENTRY("%lx %lx %x %p", memory_map_size, descriptor_size,
>  		  descriptor_version, virtmap);
>  
> +	/*
> +	 * TODO:
> +	 * Further down we are cheating. While really we should implement
> +	 * SetVirtualAddressMap() events and ConvertPointer() to allow
> +	 * dynamically loaded drivers to expose runtime services, we don't
> +	 * today.
> +	 *
> +	 * So let's ensure we see exactly one single runtime section, as
> +	 * that is the built-in one. If we see more (or less), someone must
> +	 * have tried adding or removing to that which we don't support yet.
> +	 * In that case, let's better fail rather than expose broken runtime
> +	 * services.
> +	 */
> +	for (i = 0; i < n; i++) {
> +		struct efi_mem_desc *map = (void*)virtmap +
> +					   (descriptor_size * i);
> +
> +		if (map->type == EFI_RUNTIME_SERVICES_CODE)
> +			rt_code_sections++;
> +	}
> +
> +	if (rt_code_sections != 1) {
> +		/*
> +		 * We expose exactly one single runtime code section, so
> +		 * something is definitely going wrong.
> +		 */
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	}
> +
>  	/* Rebind mmio pointers */
>  	for (i = 0; i < n; i++) {
>  		struct efi_mem_desc *map = (void*)virtmap +
> @@ -483,7 +511,7 @@ static efi_status_t EFIAPI efi_set_virtual_address_map(
>  		map = (void*)virtmap + (descriptor_size * i);
>  		if (map->type == EFI_RUNTIME_SERVICES_CODE) {
>  			ulong new_offset = map->virtual_start -
> -					   (runtime_start - gd->relocaddr);
> +					   map->physical_start + gd->relocaddr;
>  
>  			efi_runtime_relocate(new_offset, map);
>  			/* Once we're virtual, we can no longer handle
>