[U-Boot] [PATCH 1/1] efi_loader: release file buffer after loading image

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

On Tue, Mar 05, 2019 at 08:56:12PM +0100, Heinrich Schuchardt wrote:
> efi_load_pe() copies and rebases the UEFI image.
> We do not need the buffer with the file contents afterwards.
> 
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  lib/efi_loader/efi_boottime.c | 50 +++++++++++++----------------------
>  1 file changed, 18 insertions(+), 32 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_boottime.c b/lib/efi_loader/efi_boottime.c
> index bd8b8a17ae..391681260c 100644
> --- a/lib/efi_loader/efi_boottime.c
> +++ b/lib/efi_loader/efi_boottime.c
> @@ -1693,6 +1693,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>  	struct efi_loaded_image_obj **image_obj =
>  		(struct efi_loaded_image_obj **)image_handle;
>  	efi_status_t ret;
> +	void *dest_buffer;
>  
>  	EFI_ENTRY("%d, %p, %pD, %p, %zd, %p", boot_policy, parent_image,
>  		  file_path, source_buffer, source_size, image_handle);
> @@ -1708,7 +1709,7 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>  	}
>  
>  	if (!source_buffer) {
> -		ret = efi_load_image_from_path(file_path, &source_buffer,
> +		ret = efi_load_image_from_path(file_path, &dest_buffer,
>  					       &source_size);
>  		if (ret != EFI_SUCCESS)
>  			goto error;
> @@ -1721,41 +1722,26 @@ static efi_status_t EFIAPI efi_load_image(bool boot_policy,
>  		/* In this case, file_path is the "device" path, i.e.
>  		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED
>  		 */
> -		u64 addr;
> -		void *dest_buffer;
> -
> -		ret = efi_allocate_pages(EFI_ALLOCATE_ANY_PAGES,
> -					 EFI_RUNTIME_SERVICES_CODE,
> -					 efi_size_in_pages(source_size), &addr);
> -		if (ret != EFI_SUCCESS)
> -			goto error;
> -		dest_buffer = (void *)(uintptr_t)addr;
> -		memcpy(dest_buffer, source_buffer, source_size);
> -		source_buffer = dest_buffer;
> -

Are you sure? This code is what you added in your recent commit
0e18f584de59 ("efi_loader: LoadImage: always allocate new pages")
at v2019.04-rc2.

IMO, the comment:
>  		/* In this case, file_path is the "device" path, i.e.
>  		 * something like a HARDWARE_DEVICE:MEMORY_MAPPED

is also not accurate because "file_path" is normally non-NULL,
indicating where the content of "source_buffer" comes from.
In other words, "HARDWARE_DEVICE:MEMORY_MAPPED" is a quite irregular case.

-Takahiro Akashi


> +		dest_buffer = source_buffer;
>  		dp = file_path;
>  		fp = NULL;
>  	}
>  	ret = efi_setup_loaded_image(dp, fp, image_obj, &info);
> -	if (ret != EFI_SUCCESS)
> -		goto error_invalid_image;
> -	ret = efi_load_pe(*image_obj, source_buffer, info);
> -	if (ret != EFI_SUCCESS)
> -		goto error_invalid_image;
> -	/* Update the type of the allocated memory */
> -	efi_add_memory_map((uintptr_t)source_buffer,
> -			   efi_size_in_pages(source_size),
> -			   info->image_code_type, false);
> -	info->system_table = &systab;
> -	info->parent_handle = parent_image;
> -	return EFI_EXIT(EFI_SUCCESS);
> -error_invalid_image:
> -	/* The image is invalid. Release all associated resources. */
> -	efi_free_pages((uintptr_t)source_buffer,
> -		       efi_size_in_pages(source_size));
> -	efi_delete_handle(*image_handle);
> -	*image_handle = NULL;
> -	free(info);
> +	if (ret == EFI_SUCCESS)
> +		ret = efi_load_pe(*image_obj, dest_buffer, info);
> +	if (!source_buffer)
> +		/* Release buffer to which file was loaded */
> +		efi_free_pages((uintptr_t)dest_buffer,
> +			       efi_size_in_pages(source_size));
> +	if (ret == EFI_SUCCESS) {
> +		info->system_table = &systab;
> +		info->parent_handle = parent_image;
> +	} else {
> +		/* The image is invalid. Release all associated resources. */
> +		efi_delete_handle(*image_handle);
> +		*image_handle = NULL;
> +		free(info);
> +	}
>  error:
>  	return EFI_EXIT(ret);
>  }
> -- 
> 2.20.1
>