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

[Heinrich Schuchardt]

On 3/6/19 1:41 AM, AKASHI Takahiro wrote:
> 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 at 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.

I had that now rejected patch '[U-Boot,RFC,1/1] efi_loader: in situ
relocation' in mind. Maybe I should add a 'Fixes' tag pointing to
0e18f584de59.

If the caller provides a source buffer, we should not delete it. But we
there is also no need to copy it here because it is duplicated by the
relocation code.

If we read from file we need a receiving buffer, but we can delete it
once we have completed relocation.

> 
> 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.

Yes, the comment can be updated. "HARDWARE_DEVICE:MEMORY_MAPPED" is only
what we do in do_bootefi as workaround in some cases.

Regards

Heinrich

> 
> -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
>>
>