Hi Simon, Ilias,
On Thu, 9 Nov 2023 at 17:00, Ilias Apalodimas
<ilias.apalodi...@linaro.org> wrote:
>
> 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
Yes, I will fix this in next v12.
Thanks,
Masahisa Kojima
>
> 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
