On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

Hi Kojima-san,

On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:

...

This adds the URI device path option for 'boot add' subcommand. User can add the URI load option for downloading ISO image file or EFI application through network(e.g. HTTP).

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 0be3af3e76..62f867df2a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argc -= 1; argv += 1; break;

•         case 'u':
  

•         {
  

•                 char *pos;
  

•                 int uridp_len;
  

•                 struct efi_device_path_uri *uridp;
  

•                 if (argc <  3 || lo.label) {
  

•                         r = CMD_RET_USAGE;
  

•                         goto out;
  

•                 }
  

•                 id = (int)hextoul(argv[1], &endp);
  

•                 if (*endp != '\0' || id > 0xffff)
  

•                         return CMD_RET_USAGE;
  

•                 efi_create_indexed_name(var_name16, sizeof(var_name16),
  

•                                         "Boot", id);
  

•                 label = efi_convert_string(argv[2]);
  

•                 if (!label)
  

•                         return CMD_RET_FAILURE;
  

•                 lo.label = label;
  

•                 uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
  

•                 fp_free = efi_alloc(uridp_len + sizeof(END));
  

•                 uridp = (struct efi_device_path_uri *)fp_free;
  

•                 uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
  

•                 uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
  

•                 uridp->dp.length = uridp_len;
  

•                 strcpy(uridp->uri, argv[3]);
  

•                 pos = (char *)uridp + uridp_len;
  

•                 memcpy(pos, &END, sizeof(END));
  

•                 fp_size += uridp_len + sizeof(END);
  

•                 file_path = (struct efi_device_path *)uridp;
  

•                 argc -= 3;
  

•                 argv += 3;
  

•                 break;
  

•         }
  

•         default:
                  r = CMD_RET_USAGE;
                  goto out;
  

@@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" " (-b, -i for short form device path)\n"

• "  -u <bootid> <label> <uri>\n"
  

It might be a matter of personal preference, but since the current syntax of efidebug is already much complex, I don't want to add more options unless it's necessary. How about re-using "-B" option, say => efidebug -B 3 debian-netinst uri - https://foo.com/baa

I understand your concern. OK, I will add uri in -b|B and also update documentation.

Thanks, Masahisa Kojima

BTW, I think that <bootid> and <label> should have been moved out of "-b|B" when Ilias introduced this new syntax.

-Takahiro Akashi

...

  "  -s '<optional data>'\n"
  "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
  "  - delete UEFI BootXXXX variables\n"

-- 2.34.1

Hi Heinrich,

On Sat, 26 Aug 2023 at 11:20, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 8/25/23 07:57, Masahisa Kojima wrote:

...

On Thu, 24 Aug 2023 at 09:11, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

...

Hi Kojima-san,

On Wed, Aug 23, 2023 at 05:37:19PM +0900, Masahisa Kojima wrote:

...

This adds the URI device path option for 'boot add' subcommand. User can add the URI load option for downloading ISO image file or EFI application through network(e.g. HTTP).

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 0be3af3e76..62f867df2a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argc -= 1; argv += 1; break;

•         case 'u':
  

•         {
  

•                 char *pos;
  

•                 int uridp_len;
  

•                 struct efi_device_path_uri *uridp;
  

•                 if (argc <  3 || lo.label) {
  

•                         r = CMD_RET_USAGE;
  

•                         goto out;
  

•                 }
  

•                 id = (int)hextoul(argv[1], &endp);
  

•                 if (*endp != '\0' || id > 0xffff)
  

•                         return CMD_RET_USAGE;
  

•                 efi_create_indexed_name(var_name16, sizeof(var_name16),
  

•                                         "Boot", id);
  

•                 label = efi_convert_string(argv[2]);
  

•                 if (!label)
  

•                         return CMD_RET_FAILURE;
  

•                 lo.label = label;
  

•                 uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1;
  

•                 fp_free = efi_alloc(uridp_len + sizeof(END));
  

•                 uridp = (struct efi_device_path_uri *)fp_free;
  

•                 uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
  

•                 uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
  

•                 uridp->dp.length = uridp_len;
  

•                 strcpy(uridp->uri, argv[3]);
  

•                 pos = (char *)uridp + uridp_len;
  

•                 memcpy(pos, &END, sizeof(END));
  

•                 fp_size += uridp_len + sizeof(END);
  

•                 file_path = (struct efi_device_path *)uridp;
  

•                 argc -= 3;
  

•                 argv += 3;
  

•                 break;
  

•         }
  

•          default:
                   r = CMD_RET_USAGE;
                   goto out;
  

@@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" " (-b, -i for short form device path)\n"

• "  -u <bootid> <label> <uri>\n"
  

It might be a matter of personal preference, but since the current syntax of efidebug is already much complex, I don't want to add more options unless it's necessary. How about re-using "-B" option, say => efidebug -B 3 debian-netinst uri - https://foo.com/baa

You just replace one set of alternatives (-B -b -u) against another (mmc, nvme, uri, ...). I cannot see that this reduces complexity.

...

I understand your concern. OK, I will add uri in -b|B and also update documentation.

It is unclear to me what the difference between -b and -B shall be for URIs. Would -B be with the device path of the network device (/VenHw()/MAC()/URI()) and -b without (/URI())?

Differentiating -b and -B for URI is not mandatory, I think. To keep implementation simple, I would like to add '-u' option as the original patch does.

Thanks, Masahisa Kojima

Best regards

Heinrich

...

Thanks, Masahisa Kojima

...

BTW, I think that <bootid> and <label> should have been moved out of "-b|B" when Ilias introduced this new syntax.

-Takahiro Akashi

...

   "  -s '<optional data>'\n"
   "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
   "  - delete UEFI BootXXXX variables\n"

-- 2.34.1

On Thu, 24 Aug 2023 at 14:37, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

On 8/23/23 10:37, Masahisa Kojima wrote:

...

This adds the URI device path option for 'boot add' subcommand. User can add the URI load option for downloading ISO image file or EFI application through network(e.g. HTTP).

Signed-off-by: Masahisa Kojima masahisa.kojima@linaro.org

cmd/efidebug.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)

diff --git a/cmd/efidebug.c b/cmd/efidebug.c index 0be3af3e76..62f867df2a 100644 --- a/cmd/efidebug.c +++ b/cmd/efidebug.c @@ -829,6 +829,44 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int flag, argc -= 1; argv += 1; break;

•         case 'u':
  

•         {
  

•                 char *pos;
  

•                 int uridp_len;
  

•                 struct efi_device_path_uri *uridp;
  

•                 if (argc <  3 || lo.label) {
  

•                         r = CMD_RET_USAGE;
  

•                         goto out;
  

•                 }
  

•                 id = (int)hextoul(argv[1], &endp);
  

•                 if (*endp != '\0' || id > 0xffff)
  

•                         return CMD_RET_USAGE;
  

•                 efi_create_indexed_name(var_name16, sizeof(var_name16),
  

•                                         "Boot", id);
  

•                 label = efi_convert_string(argv[2]);
  

•                 if (!label)
  

•                         return CMD_RET_FAILURE;
  

•                 lo.label = label;
  

•                 uridp_len = sizeof(struct efi_device_path) + strlen(argv[3]) + 1; > +                   fp_free = efi_alloc(uridp_len + sizeof(END));
  

•                 uridp = (struct efi_device_path_uri *)fp_free;
  

•                 uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
  

•                 uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
  

•                 uridp->dp.length = uridp_len;
  

•                 strcpy(uridp->uri, argv[3]);
  

This assumes that argv[3] is a valid URI.

Would it be preferable to validate that the string is percent encoded, conforms to RFC 3986, and contains a supported scheme, an authority, and a path?

As a user I would like related errors to be caught at entry and not at runtime.

OK, I agree. I will add input uri validation here.

Thanks, Masahisa Kojima

Best regards

Heinrich

...

•                 pos = (char *)uridp + uridp_len;
  

•                 memcpy(pos, &END, sizeof(END));
  

•                 fp_size += uridp_len + sizeof(END);
  

•                 file_path = (struct efi_device_path *)uridp;
  

•                 argc -= 3;
  

•                 argv += 3;
  

•                 break;
  

•         }
  

•         default:
                  r = CMD_RET_USAGE;
                  goto out;
  

@@ -1492,6 +1530,7 @@ static char efidebug_help_text[] = " -b|-B <bootid> <label> <interface> <devnum>[:<part>] <file path>\n" " -i|-I <interface> <devnum>[:<part>] <initrd file path>\n" " (-b, -i for short form device path)\n"

• "  -u <bootid> <label> <uri>\n"
  "  -s '<optional data>'\n"
  "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n"
  "  - delete UEFI BootXXXX variables\n"
  

Hi Masahisa,

On Wed, 23 Aug 2023 at 02:38, Masahisa Kojima masahisa.kojima@linaro.org 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@linaro.org

lib/efi_loader/efi_bootmgr.c | 213 +++++++++++++++++++++++++++++++++++ 1 file changed, 213 insertions(+)

Much of this code should be factored out into a help which can be called from other code. See comments below.

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;
  

From here:

•   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. */
  

•   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. */
  

•   blk_list_part(UCLASS_BLKMAP);
  

•   /*
  

•    * 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)
  

•   {
  

check code style

•           device_foreach_child(partition, blk)
  

•           {
  

to here.

Perhaps have partition_first() and partition_next() iterators?

From here is EFI code:

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

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

•           }
  

•   }
  

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

From here should be normal caode:

•   /*
  

•    * 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, "/");
  

•   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) {
  

•           log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
  

•           ret = EFI_INVALID_PARAMETER;
  

•           goto out;
  

•   }
  

•   s = env_get("httpserverip");
  

•   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() */
  

•   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);
  

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

•    */
  

•   file_name_len = strlen(net_boot_file_name);
  

to here.

You could package that up into a function which returns the filename. Then the EFI code is this:

•   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)) {
  

P> + 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, -- 2.34.1

Also please consider how to test this. With the changes I mention above, you can fairly easily test the partition stuff (we have several mmc images for testing).

Regards, Simon

Kojima-san,

On Wed, Aug 23, 2023 at 05:37:20PM +0900, 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.

Is this behavior part of UEFI specification? Even so, it would be better to describe it in uefi.rst (or else), including URI usage.

Signed-off-by: Masahisa Kojima masahisa.kojima@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)
  

I wonder why not adding "address" parameter to make this function more generic as Simon suggested.

+{

• 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. */
• 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. */
• blk_list_part(UCLASS_BLKMAP);

I think that blkmap should provide a way to *probe* its child block device. In fact, struct blkmap *bm = dev_get_plat(bm_dev); device_probe(bm->blk); should work. (Not tested though)

• /*

• * 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)

Does bm_dev have more than one block devices?

• {

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

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

• }
  

• }

Another idea would be to create a new boot option (as a removable media) and to invoke it from bootmgr.

+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, "/");
• 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) {

• log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
  

• ret = EFI_INVALID_PARAMETER;
  

• goto out;
  

• }
• s = env_get("httpserverip");
• 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() */
• 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);
• 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.
  

• */
  

• 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);
  

Nak. try_load_from_uri_path() is called from try_load_entry(). This function's purpose is solely to load an image. The execution should be triggered later.

-Takahiro Akashi

• } 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, -- 2.34.1

On Thu, 24 Aug 2023 at 11:24, AKASHI Takahiro takahiro.akashi@linaro.org wrote:

Kojima-san,

On Wed, Aug 23, 2023 at 05:37:20PM +0900, 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.

Is this behavior part of UEFI specification? Even so, it would be better to describe it in uefi.rst (or else), including URI usage.

Yes, I will update the documentation.

...

Signed-off-by: Masahisa Kojima masahisa.kojima@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)
  

I wonder why not adding "address" parameter to make this function more generic as Simon suggested.

wget and other network commands take the load address with the image_load_address global variable. Anyway, adding address as parameter is better.

...

+{

• 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. */
  

• 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. */
  

• blk_list_part(UCLASS_BLKMAP);
  

I think that blkmap should provide a way to *probe* its child block device. In fact, struct blkmap *bm = dev_get_plat(bm_dev); device_probe(bm->blk); should work. (Not tested though)

Thank you. I implemented this initially but did not work as expected. I will check again.

...

• /*
  

•  * 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)
  

Does bm_dev have more than one block devices?

No, I can remove foreach_child.

...

• {
  

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

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

•         }
  

• }
  

Another idea would be to create a new boot option (as a removable media) and to invoke it from bootmgr.

...

+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, "/");
  

• 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) {
  

•         log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
  

•         ret = EFI_INVALID_PARAMETER;
  

•         goto out;
  

• }
  

• s = env_get("httpserverip");
  

• 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() */
  

• 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);
  

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

•  */
  

• 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);
  

Nak. try_load_from_uri_path() is called from try_load_entry(). This function's purpose is solely to load an image. The execution should be triggered later.

Yes, you are correct. I will fix this.

Thanks, Masahisa Kojima

-Takahiro Akashi

...

• } 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, -- 2.34.1

On Thu, 24 Aug 2023 at 15:57, Heinrich Schuchardt xypron.glpk@gmx.de wrote:

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

I have tried but I can't get block size. Anyway I will check again.

...

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

Thank you for correcting the typo.

...

• blk_list_part(UCLASS_BLKMAP);
  

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

I will try to probe all the partitions with device_probe() call.

...

• /*
  

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

OK.

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

I will check how to handle this case.

...

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

According to the UEFI spec, the system can boot from the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL. This loop does not try to find the ESP, try to find the device having EFI_SIMPLE_FILE_SYSTEM_PROTOCOL and check if there is a default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).

...

•                 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

At here, the default boot file is loaded into the memory, we could exit the loop and start the image.

...

•         }
  

• }
  

ret may not even be initialized at this point!

Thank you, I will fix this, EFI_NOT_FOUND should be returned.

I would expect EFI_NOT_FOUND to be returned if there is no ESP.

My intention here is that there is no bootable device contains default boot file(e.g. EFI/BOOT/BOOTAA64.EFI).

...

+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@example.com".

Yes, but current wget does not support this format. Need to be checked when the user input the URI as you suggested.

...

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

Currently lwip migration is ongoing, and lwip wget correctly handles this. I plan to rebase on top of the lwip port.

...

•         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"?

OK.

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

OK.

...

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

Current wget does not support HOST: header but lwip port does support. So lwip migration will address this issue.

...

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

OK, I will add the maximum file size env variable, or try to utilize the Content-Length response header.

...

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

OK.

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

OK.

Thanks, Masahisa Kojima

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,