[U-Boot,6/8] efi_loader: Pass file path to payload

The payload gets information on where it got loaded from. This includes
the device as well as file path.

So far we've treated both as the same thing and always gave it the device
name. However, in some situations grub2 actually wants to find its loading
path to find its configuration file.

So let's split the two semantically separte bits into separate structs and
pass the loaded file name into our payload when we load it using "load".

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 cmd/bootefi.c        | 32 +++++++++++++++++++++++++-------
 cmd/fs.c             |  3 ++-
 include/efi_loader.h |  5 +++--
 3 files changed, 30 insertions(+), 10 deletions(-)

On Mon, Apr 11, 2016 at 04:16:19PM +0200, Alexander Graf wrote:

> The payload gets information on where it got loaded from. This includes
> the device as well as file path.
> 
> So far we've treated both as the same thing and always gave it the device
> name. However, in some situations grub2 actually wants to find its loading
> path to find its configuration file.
> 
> So let's split the two semantically separte bits into separate structs and
> pass the loaded file name into our payload when we load it using "load".
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>

Applied to u-boot/master, thanks!

On 04/11/2016 04:16 PM, Alexander Graf wrote:
> The payload gets information on where it got loaded from. This includes
> the device as well as file path.
> 
> So far we've treated both as the same thing and always gave it the device
> name. However, in some situations grub2 actually wants to find its loading
> path to find its configuration file.
> 
> So let's split the two semantically separte bits into separate structs and
> pass the loaded file name into our payload when we load it using "load".
> 
> Signed-off-by: Alexander Graf <agraf@suse.de>
> ---
>  cmd/bootefi.c        | 32 +++++++++++++++++++++++++-------
>  cmd/fs.c             |  3 ++-
>  include/efi_loader.h |  5 +++--
>  3 files changed, 30 insertions(+), 10 deletions(-)
> 
> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
> index 0d09aa1..adcf645 100644
> --- a/cmd/bootefi.c
> +++ b/cmd/bootefi.c
> @@ -34,17 +34,30 @@ static struct efi_device_path_file_path bootefi_image_path[] = {
>  	}
>  };
>  
> +static struct efi_device_path_file_path bootefi_device_path[] = {
> +	{
> +		.dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE,
> +		.dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH,
> +		.dp.length = sizeof(bootefi_image_path[0]),
> +		.str = { 'b','o','o','t','e','f','i' },
> +	}, {
> +		.dp.type = DEVICE_PATH_TYPE_END,
> +		.dp.sub_type = DEVICE_PATH_SUB_TYPE_END,
> +		.dp.length = sizeof(bootefi_image_path[0]),
> +	}
> +};
> +
>  static efi_status_t bootefi_open_dp(void *handle, efi_guid_t *protocol,
>  			void **protocol_interface, void *agent_handle,
>  			void *controller_handle, uint32_t attributes)
>  {
> -	*protocol_interface = bootefi_image_path;
> +	*protocol_interface = bootefi_device_path;
>  	return EFI_SUCCESS;
>  }
>  
>  /* The EFI loaded_image interface for the image executed via "bootefi" */
>  static struct efi_loaded_image loaded_image_info = {
> -	.device_handle = bootefi_image_path,
> +	.device_handle = bootefi_device_path,
>  	.file_path = bootefi_image_path,
>  };
>  
> @@ -63,7 +76,7 @@ static struct efi_object loaded_image_info_obj = {
>  		{
>  			/*
>  			 * When asking for the device path interface, return
> -			 * bootefi_image_path
> +			 * bootefi_device_path
>  			 */
>  			.guid = &efi_guid_device_path,
>  			.open = &bootefi_open_dp,
> @@ -73,11 +86,11 @@ static struct efi_object loaded_image_info_obj = {
>  
>  /* The EFI object struct for the device the "bootefi" image was loaded from */
>  static struct efi_object bootefi_device_obj = {
> -	.handle = bootefi_image_path,
> +	.handle = bootefi_device_path,
>  	.protocols = {
>  		{
>  			/* When asking for the device path interface, return
> -			 * bootefi_image_path */
> +			 * bootefi_device_path */
>  			.guid = &efi_guid_device_path,
>  			.open = &bootefi_open_dp,
>  		}
> @@ -192,7 +205,7 @@ U_BOOT_CMD(
>  	bootefi_help_text
>  );
>  
> -void efi_set_bootdev(const char *dev, const char *devnr)
> +void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>  {
>  	__maybe_unused struct blk_desc *desc;
>  	char devname[16] = { 0 }; /* dp->str is u16[16] long */
> @@ -217,7 +230,12 @@ void efi_set_bootdev(const char *dev, const char *devnr)
>  	if (colon)
>  		*colon = '\0';
>  
> -	/* Patch the bootefi_image_path to the target device */
> +	/* Patch bootefi_device_path to the target device */
> +	memset(bootefi_device_path[0].str, 0, sizeof(bootefi_device_path[0].str));
> +	ascii2unicode(bootefi_device_path[0].str, devname);
> +
> +	/* Patch bootefi_image_path to the target file path */
>  	memset(bootefi_image_path[0].str, 0, sizeof(bootefi_image_path[0].str));
> +	snprintf(devname, sizeof(devname), "%s", path);
>  	ascii2unicode(bootefi_image_path[0].str, devname);
>  }
> diff --git a/cmd/fs.c b/cmd/fs.c
> index be8f289..abfe5be 100644
> --- a/cmd/fs.c
> +++ b/cmd/fs.c
> @@ -27,7 +27,8 @@ U_BOOT_CMD(
>  static int do_load_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
>  				char * const argv[])
>  {
> -	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "");
> +	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
> +			(argc > 4) ? argv[4] : "");

Hi Alex,

efi_set_bootdev is used here to set both the device from which a file
was loaded and the filename used as image_path.

Why should we assume that the last file loaded is the EFI executable and
not the device tree (or any other file)?

Shouldn't we at least check that this file is an EFI executable or driver?

Best regards

Heinrich

>  	return do_load(cmdtp, flag, argc, argv, FS_TYPE_ANY);
>  }
>  
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 9f61fc4..88b8149 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -112,7 +112,7 @@ efi_status_t efi_exit_func(efi_status_t ret);
>  /* Call this to relocate the runtime section to an address space */
>  void efi_runtime_relocate(ulong offset, struct efi_mem_desc *map);
>  /* Call this to set the current device name */
> -void efi_set_bootdev(const char *dev, const char *devnr);
> +void efi_set_bootdev(const char *dev, const char *devnr, const char *path);
>  
>  /* Generic EFI memory allocator, call this to get memory */
>  void *efi_alloc(uint64_t len, int memory_type);
> @@ -155,6 +155,7 @@ static inline void ascii2unicode(u16 *unicode, char *ascii)
>  
>  /* No loader configured, stub out EFI_ENTRY */
>  static inline void efi_restore_gd(void) { }
> -static inline void efi_set_bootdev(const char *dev, const char *devnr) { }
> +static inline void efi_set_bootdev(const char *dev, const char *devnr,
> +				   const char *path) { }
>  
>  #endif
>

On 08/13/2018 09:21 PM, Heinrich Schuchardt wrote:
> On 04/11/2016 04:16 PM, Alexander Graf wrote:
>> The payload gets information on where it got loaded from. This includes
>> the device as well as file path.
>>
>> So far we've treated both as the same thing and always gave it the device
>> name. However, in some situations grub2 actually wants to find its loading
>> path to find its configuration file.
>>
>> So let's split the two semantically separte bits into separate structs and
>> pass the loaded file name into our payload when we load it using "load".
>>
>> Signed-off-by: Alexander Graf <agraf@suse.de>
>> ---
>>   cmd/bootefi.c        | 32 +++++++++++++++++++++++++-------
>>   cmd/fs.c             |  3 ++-
>>   include/efi_loader.h |  5 +++--
>>   3 files changed, 30 insertions(+), 10 deletions(-)
>>
>> diff --git a/cmd/bootefi.c b/cmd/bootefi.c
>> index 0d09aa1..adcf645 100644
>> --- a/cmd/bootefi.c
>> +++ b/cmd/bootefi.c
>> @@ -34,17 +34,30 @@ static struct efi_device_path_file_path bootefi_image_path[] = {
>>   	}
>>   };
>>   
>> +static struct efi_device_path_file_path bootefi_device_path[] = {
>> +	{
>> +		.dp.type = DEVICE_PATH_TYPE_MEDIA_DEVICE,
>> +		.dp.sub_type = DEVICE_PATH_SUB_TYPE_FILE_PATH,
>> +		.dp.length = sizeof(bootefi_image_path[0]),
>> +		.str = { 'b','o','o','t','e','f','i' },
>> +	}, {
>> +		.dp.type = DEVICE_PATH_TYPE_END,
>> +		.dp.sub_type = DEVICE_PATH_SUB_TYPE_END,
>> +		.dp.length = sizeof(bootefi_image_path[0]),
>> +	}
>> +};
>> +
>>   static efi_status_t bootefi_open_dp(void *handle, efi_guid_t *protocol,
>>   			void **protocol_interface, void *agent_handle,
>>   			void *controller_handle, uint32_t attributes)
>>   {
>> -	*protocol_interface = bootefi_image_path;
>> +	*protocol_interface = bootefi_device_path;
>>   	return EFI_SUCCESS;
>>   }
>>   
>>   /* The EFI loaded_image interface for the image executed via "bootefi" */
>>   static struct efi_loaded_image loaded_image_info = {
>> -	.device_handle = bootefi_image_path,
>> +	.device_handle = bootefi_device_path,
>>   	.file_path = bootefi_image_path,
>>   };
>>   
>> @@ -63,7 +76,7 @@ static struct efi_object loaded_image_info_obj = {
>>   		{
>>   			/*
>>   			 * When asking for the device path interface, return
>> -			 * bootefi_image_path
>> +			 * bootefi_device_path
>>   			 */
>>   			.guid = &efi_guid_device_path,
>>   			.open = &bootefi_open_dp,
>> @@ -73,11 +86,11 @@ static struct efi_object loaded_image_info_obj = {
>>   
>>   /* The EFI object struct for the device the "bootefi" image was loaded from */
>>   static struct efi_object bootefi_device_obj = {
>> -	.handle = bootefi_image_path,
>> +	.handle = bootefi_device_path,
>>   	.protocols = {
>>   		{
>>   			/* When asking for the device path interface, return
>> -			 * bootefi_image_path */
>> +			 * bootefi_device_path */
>>   			.guid = &efi_guid_device_path,
>>   			.open = &bootefi_open_dp,
>>   		}
>> @@ -192,7 +205,7 @@ U_BOOT_CMD(
>>   	bootefi_help_text
>>   );
>>   
>> -void efi_set_bootdev(const char *dev, const char *devnr)
>> +void efi_set_bootdev(const char *dev, const char *devnr, const char *path)
>>   {
>>   	__maybe_unused struct blk_desc *desc;
>>   	char devname[16] = { 0 }; /* dp->str is u16[16] long */
>> @@ -217,7 +230,12 @@ void efi_set_bootdev(const char *dev, const char *devnr)
>>   	if (colon)
>>   		*colon = '\0';
>>   
>> -	/* Patch the bootefi_image_path to the target device */
>> +	/* Patch bootefi_device_path to the target device */
>> +	memset(bootefi_device_path[0].str, 0, sizeof(bootefi_device_path[0].str));
>> +	ascii2unicode(bootefi_device_path[0].str, devname);
>> +
>> +	/* Patch bootefi_image_path to the target file path */
>>   	memset(bootefi_image_path[0].str, 0, sizeof(bootefi_image_path[0].str));
>> +	snprintf(devname, sizeof(devname), "%s", path);
>>   	ascii2unicode(bootefi_image_path[0].str, devname);
>>   }
>> diff --git a/cmd/fs.c b/cmd/fs.c
>> index be8f289..abfe5be 100644
>> --- a/cmd/fs.c
>> +++ b/cmd/fs.c
>> @@ -27,7 +27,8 @@ U_BOOT_CMD(
>>   static int do_load_wrapper(cmd_tbl_t *cmdtp, int flag, int argc,
>>   				char * const argv[])
>>   {
>> -	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "");
>> +	efi_set_bootdev(argv[1], (argc > 2) ? argv[2] : "",
>> +			(argc > 4) ? argv[4] : "");
> Hi Alex,
>
> efi_set_bootdev is used here to set both the device from which a file
> was loaded and the filename used as image_path.
>
> Why should we assume that the last file loaded is the EFI executable and
> not the device tree (or any other file)?

The image path logic goes hand in hand with the distro boot logic which 
tries to always load the efi binary last. If you found a case where it 
doesn't, let's fix that up please.

> Shouldn't we at least check that this file is an EFI executable or driver?

Well, that would only be a marginal improvement, no? And in fact I'm not 
sure I'd be very happy to clutter the load code with such logic.

The "real" fix would be to create a LoadedImage object for every "load" 
call and then just search for the one that starts at the bootefi load 
address. But I'm also not sure people who don't want to run any efi 
payloads want to pay the memory penalty that incurs.


Alex