[PATCH 2/2] efi_loader: support boot from URI device path

[Heinrich Schuchardt]

On 8/23/23 10:37, Masahisa Kojima wrote:
> This supports to boot from the URI device path.
> When user selects the URI device path, bootmgr downloads
> the file using wget into the address specified by loadaddr
> env variable.
> If the file is .iso or .img file, mount the image with blkmap
> then try to boot with the default file(e.g. EFI/BOOT/BOOTAA64.EFI).
> If the file is .efi file, load and start the downloaded file.
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> ---
>   lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++
>   1 file changed, 213 insertions(+)
>
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index a40762c74c..8b20f486f2 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -7,10 +7,14 @@
>
>   #define LOG_CATEGORY LOGC_EFI
>
> +#include <blk.h>
> +#include <blkmap.h>
>   #include <common.h>
>   #include <charset.h>
> +#include <dm.h>
>   #include <log.h>
>   #include <malloc.h>
> +#include <net.h>
>   #include <efi_default_filename.h>
>   #include <efi_loader.h>
>   #include <efi_variable.h>
> @@ -168,6 +172,209 @@ out:
>   	return ret;
>   }
>
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +/**
> + * mount_image() - mount the image
> + *
> + * @lo_label	label of load option
> + * @file_size	file size
> + * @handle:	pointer to handle for newly installed image
> + * Return:	status code
> + */
> +static efi_status_t mount_image(u16 *lo_label, int file_size,
> +				efi_handle_t *handle)
> +{
> +	int err;
> +	efi_status_t ret;
> +	char *label = NULL, *p;
> +	lbaint_t blknum;
> +	struct udevice *bm_dev;
> +	efi_handle_t bm_handle;
> +	struct udevice *blk, *partition;
> +	struct efi_handler *handler;
> +	struct efi_device_path *file_path;
> +	struct efi_device_path *device_path;
> +
> +	label = efi_alloc(utf16_utf8_strlen(lo_label) + 1);
> +	if (!label)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	p = label;
> +	utf16_utf8_strcpy(&p, lo_label);
> +	err = blkmap_create(label, NULL);
> +	if (err) {
> +		log_err("failed to create blkmap\n");
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	bm_dev = blkmap_from_label(label);
> +	if (!bm_dev) {
> +		log_err("\"%s\" is not the name of any known blkmap\n", label);
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	blknum = file_size / 512; /* TODO: don't use literal value. */

Can't you retrieve the block size from the udevice?

> +	err = blkmap_map_pmem(bm_dev, 0, blknum, image_load_addr);
> +	if (err) {
> +		log_err("Unable to map %#llx at block %d : %d\n",
> +			(unsigned long long)image_load_addr, 0, err);
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	log_info("Block %d+0x" LBAF " mapped to %#llx\n", 0, blknum,
> +		 (unsigned long long)image_load_addr);
> +
> +	/* TODO: without calling this, partition devices are not binded. */

%s/binded/bound/

> +	blk_list_part(UCLASS_BLKMAP);

Why would you want to display all BLKMAP devices?
Please, avoid unnecessary output.

> +
> +	/*
> +	 * Search the partition having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL,
> +	 * then try to load with the default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).
> +	 */
> +	device_foreach_child(blk, bm_dev)
> +	{

You need to check that blk is of type UCLASS_PARTITION.

What about images that have no partition table but only a file system?

> +		device_foreach_child(partition, blk)
> +		{
> +			if (dev_tag_get_ptr(partition, DM_TAG_EFI,
> +					    (void **)&bm_handle)) {
> +				log_warning("DM_TAG_EFI not found\n");
> +				continue;
> +			}
> +
> +			ret = efi_search_protocol(
> +				bm_handle,
> +				&efi_simple_file_system_protocol_guid,
> +				&handler);
> +			if (ret != EFI_SUCCESS)
> +				continue;
> +
> +			ret = efi_search_protocol(
> +				bm_handle, &efi_guid_device_path, &handler);
> +			if (ret != EFI_SUCCESS)
> +				continue;
> +
> +			ret = efi_protocol_open(handler, (void **)&device_path,
> +						efi_root, NULL,
> +						EFI_OPEN_PROTOCOL_GET_PROTOCOL);
> +			if (ret != EFI_SUCCESS)
> +				continue;

Do you expect multiple ESPs? Why not return the error here?

> +
> +			file_path = expand_media_path(device_path);
> +			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,
> +						      NULL, 0, handle));
> +			efi_free_pool(file_path);
> +			if (ret == EFI_SUCCESS)
> +				goto out;

ditto

> +		}
> +	}
> +

ret may not even be initialized at this point!
I would expect EFI_NOT_FOUND to be returned if there is no ESP.

> +out:
> +	efi_free_pool(label);
> +
> +	return ret;
> +}
> +
> +/**
> + * try_load_from_uri_path() - Handle the URI device path
> + *
> + * @uridp:	uri device path
> + * @lo_label	label of load option
> + * @handle:	pointer to handle for newly installed image
> + * Return:	status code
> + */
> +static efi_status_t try_load_from_uri_path(struct efi_device_path_uri *uridp,
> +					   u16 *lo_label,
> +					   efi_handle_t *handle)
> +{
> +	efi_status_t ret;
> +	int file_size, file_name_len;
> +	char *s, *host_name, *file_name, *str_copy;
> +
> +	/*
> +	 * Download file using wget.
> +	 *
> +	 * URI device path content is like http://www.example.com/sample/test.iso.
> +	 * U-Boot wget takes the target uri in this format.
> +	 *  "<http server ip>:<file path>"  e.g.) 192.168.1.1:/sample/test.iso
> +	 * Need to resolve the http server ip address before starting wget.
> +	 */
> +
> +	/* only support "http://" */
> +	if (strncmp(uridp->uri, "http://", 7)) {
> +		log_err("Error: uri must start with http://\n");
> +		return EFI_INVALID_PARAMETER;
> +	}
> +
> +	str_copy = strdup(uridp->uri);
> +	if (!str_copy)
> +		return EFI_OUT_OF_RESOURCES;
> +	s = str_copy + strlen("http://");
> +	host_name = strsep(&s, "/");

This could be "user:password at example.com".

> +	if (!s) {
> +		log_err("Error: invalied uri, no file path\n");
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	file_name = s;
> +	net_dns_resolve = host_name;
> +	net_dns_env_var = "httpserverip";
> +	if (net_loop(DNS) < 0) {

Why call net_loop(DNS) for an IP address like
[2a00:1450:4001:812::200e] or 142.250.185.206?

> +		log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}

This logic seems not to be EFI related. There should be a network
library function that takes a URL and returns a filled buffer.

> +	s = env_get("httpserverip");

Why should this variable be used if host_name is "142.250.185.206"?

If the host name has no DNS entry and is not a valid IP address we
should error out here.

> +	if (!s) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	/*
> +	 * WGET requires that "net_boot_file_name" and "image_load_addr" global
> +	 * variables are properly set in advance.
> +	 */
> +	strlcpy(net_boot_file_name, s, 1024);
> +	strlcat(net_boot_file_name, ":/", 1024); /* append '/' which is removed by strsep() */

On a single IP address you may find multiple servers. Even if there is
only one it may not provide the resource if you don't supply the host name.

It would be preferable to adjust wget to comply to RFC 7320 ("Hypertext
Transfer Protocol (HTTP/1.1): Message Syntax and Routing") and provide a
HOST: header.

> +	strlcat(net_boot_file_name, file_name, 1024);
> +	s = env_get("loadaddr");
> +	if (!s) {
> +		log_err("Error: loadaddr is not set\n");
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	image_load_addr = hextoul(s, NULL);
> +
> +	file_size = net_loop(WGET);

This looks insecure.

You must define a maximum file size before trying to download and use
lmb_init_and_reserve() to check that the buffer is available. Otherwise
you might download a large file that overwrites the stack or U-Boot's code.

net_loop() must check that the reserved memory size is not exceeded.

> +	if (file_size < 0) {
> +		log_err("Error: downloading file failed\n");
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Identify file type by file extension.
> +	 * If the file extension is ".iso" or ".img", mount it and boot with default file.
> +	 * If the file is ".efi", load and start the downloaded file.

Please, don't rely on file extensions.

Inspect the buffer using function efi_check_pe() to discover if it is an
EFI binary.

mount_image() should return an error code if the buffer does not contain
a partition table or a file system.

Best regards

Heinrich

> +	 */
> +	file_name_len = strlen(net_boot_file_name);
> +	if (!strncmp(&net_boot_file_name[file_name_len - 4], ".iso", 4) ||
> +	    !strncmp(&net_boot_file_name[file_name_len - 4], ".img", 4)) {
> +		ret = mount_image(lo_label, file_size, handle);
> +	} else if (!strncmp(&net_boot_file_name[file_name_len - 4], ".efi", 4)) {
> +		ret = efi_run_image((void *)image_load_addr, file_size);
> +	} else {
> +		log_err("Error: file type is not supported\n");
> +		ret = EFI_INVALID_PARAMETER;
> +	}
> +
> +out:
> +	free(str_copy);
> +
> +	return ret;
> +}
> +#endif
> +
>   /**
>    * try_load_entry() - try to load image for boot option
>    *
> @@ -211,6 +418,12 @@ static efi_status_t try_load_entry(u16 n, efi_handle_t *handle,
>   		if (EFI_DP_TYPE(lo.file_path, MEDIA_DEVICE, FILE_PATH)) {
>   			/* file_path doesn't contain a device path */
>   			ret = try_load_from_short_path(lo.file_path, handle);
> +#if (IS_ENABLED(CONFIG_BLKMAP) && IS_ENABLED(CONFIG_CMD_WGET) && IS_ENABLED(CONFIG_CMD_DNS))
> +		} else if (EFI_DP_TYPE(lo.file_path, MESSAGING_DEVICE, MSG_URI)) {
> +			ret = try_load_from_uri_path(
> +				(struct efi_device_path_uri *)lo.file_path,
> +				lo.label, handle);
> +#endif
>   		} else {
>   			file_path = expand_media_path(lo.file_path);
>   			ret = EFI_CALL(efi_load_image(true, efi_root, file_path,