Hi Simon, On Thu, 9 Nov 2023 at 03:15, Simon Glass <s...@chromium.org> wrote: > > Hi Masahisa, > > On Wed, 8 Nov 2023 at 04:08, Masahisa Kojima <masahisa.koj...@linaro.org> > 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. Currently HTTP is only supported. > > > > Signed-off-by: Masahisa Kojima <masahisa.koj...@linaro.org> > > Acked-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > > --- > > cmd/efidebug.c | 51 ++++++++++++++++++++++++++++++++++++ > > include/net.h | 8 ++++++ > > net/wget.c | 71 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 130 insertions(+) > > Reviewed-by: Simon Glass <s...@chromium.org> > > > > > diff --git a/cmd/efidebug.c b/cmd/efidebug.c > > index 201531ac19..42b306e708 100644 > > --- a/cmd/efidebug.c > > +++ b/cmd/efidebug.c > > @@ -19,6 +19,7 @@ > > #include <log.h> > > #include <malloc.h> > > #include <mapmem.h> > > +#include <net.h> > > #include <part.h> > > #include <search.h> > > #include <linux/ctype.h> > > @@ -829,6 +830,53 @@ static int do_efi_boot_add(struct cmd_tbl *cmdtp, int > > flag, > > argc -= 1; > > argv += 1; > > break; > > +#if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) > > (my very late comment could be looked at after this is applied, since > it is already at v11) > > Can this be move to 'if'? You can put it inside the case and really > this code should be in a function, as do_efi_boot_add() is already > long.
I think it can. Kojima-san will send a v12 since there's a small issue in patch #7. I think he can fix that as well Thanks /Ilias > > > + 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; > > + > > + label = efi_convert_string(argv[2]); > > + if (!label) > > + return CMD_RET_FAILURE; > > + > > + if (!wget_validate_uri(argv[3])) { > > + printf("ERROR: invalid URI\n"); > > + r = CMD_RET_FAILURE; > > + goto out; > > + } > > + > > + efi_create_indexed_name(var_name16, > > sizeof(var_name16), > > + "Boot", id); > > + lo.label = label; > > + > > + uridp_len = sizeof(struct efi_device_path) + > > strlen(argv[3]) + 1; > > + fp_size += uridp_len + sizeof(END); > > + fp_free = efi_alloc(fp_size); > > + 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)); > > + file_path = (struct efi_device_path *)uridp; > > + argc -= 3; > > + argv += 3; > > + break; > > + } > > +#endif > > + > > default: > > r = CMD_RET_USAGE; > > goto out; > > @@ -1491,6 +1539,9 @@ U_BOOT_LONGHELP(efidebug, > > " -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" > > +#if (IS_ENABLED(CONFIG_EFI_HTTP_BOOT)) > > + " -u <bootid> <label> <uri>\n" > > +#endif > > " -s '<optional data>'\n" > > "efidebug boot rm <bootid#1> [<bootid#2> [<bootid#3> [...]]]\n" > > " - delete UEFI BootXXXX variables\n" > > diff --git a/include/net.h b/include/net.h > > index 57889d8b7a..c748974573 100644 > > --- a/include/net.h > > +++ b/include/net.h > > @@ -935,4 +935,12 @@ static inline void eth_set_enable_bootdevs(bool > > enable) {} > > */ > > int wget_with_dns(ulong dst_addr, char *uri); > > > > +/** > > + * wget_validate_uri() - varidate the uri > > + * > > + * @uri: uri string of target file of wget > > + * Return: true if uri is valid, false if uri is invalid > > + */ > > +bool wget_validate_uri(char *uri); > > + > > #endif /* __NET_H__ */ > > diff --git a/net/wget.c b/net/wget.c > > index 2087146b37..6ae2237a0a 100644 > > --- a/net/wget.c > > +++ b/net/wget.c > > @@ -566,3 +566,74 @@ out: > > return ret; > > } > > #endif > > + > > +/** > > + * wget_validate_uri() - validate the uri for wget > > + * > > + * @uri: uri string > > + * > > + * This function follows the current U-Boot wget implementation. > > + * scheme: only "http:" is supported > > + * authority: > > + * - user information: not supported > > + * - host: supported > > + * - port: not supported(always use the default port) > > + * > > + * Uri is expected to be correctly percent encoded. > > + * This is the minimum check, control codes(0x1-0x19, 0x7F, except '\0') > > + * and space character(0x20) are not allowed. > > + * > > + * TODO: stricter uri conformance check > > + * > > + * Return: true on success, false on failure > > + */ > > +bool wget_validate_uri(char *uri) > > +{ > > + char c; > > + bool ret = true; > > + char *str_copy, *s, *authority; > > + > > + for (c = 0x1; c < 0x21; c++) { > > + if (strchr(uri, c)) { > > + log_err("invalid character is used\n"); > > + return false; > > + } > > + } > > + if (strchr(uri, 0x7f)) { > > + log_err("invalid character is used\n"); > > + return false; > > + } > > + > > + if (strncmp(uri, "http://", 7)) { > > + log_err("only http:// is supported\n"); > > + return false; > > + } > > + str_copy = strdup(uri); > > + if (!str_copy) > > + return false; > > + > > + s = str_copy + strlen("http://"); > > + authority = strsep(&s, "/"); > > + if (!s) { > > + log_err("invalid uri, no file path\n"); > > + ret = false; > > + goto out; > > + } > > + s = strchr(authority, '@'); > > + if (s) { > > + log_err("user information is not supported\n"); > > + ret = false; > > + goto out; > > + } > > + s = strchr(authority, ':'); > > + if (s) { > > + log_err("user defined port is not supported\n"); > > + ret = false; > > + goto out; > > + } > > + > > +out: > > + free(str_copy); > > + > > + return ret; > > +} > > -- > > 2.34.1 > > > > Regards, > Simon