diff mbox series

cmd: eficonfig: add support for URI device path based boot options

Message ID 20250616102338.1269541-1-sughosh.ganu@linaro.org
State New
Headers show
Series cmd: eficonfig: add support for URI device path based boot options | expand

Commit Message

Sughosh Ganu June 16, 2025, 10:23 a.m. UTC
The eficonfig command provides a menu based interface for maintenance
of the EFI boot options. Add support for adding a URI based boot
option. This boot option can then be used for HTTP boot.

Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
---

Note: I had explored adding a corresponding test for this change. But
looks like the eficonfig command itself is not being tested since
commit 952018117ab4 ("dm: sandbox: Switch over to using the new host
uclass"). This needs to be taken up separately as to why the eficonfig
test has been bypassed, before adding stuff to the test.


 cmd/eficonfig.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 87 insertions(+), 3 deletions(-)

Comments

Heinrich Schuchardt June 16, 2025, 9:53 p.m. UTC | #1
On 6/16/25 12:23, Sughosh Ganu wrote:
> The eficonfig command provides a menu based interface for maintenance
> of the EFI boot options. Add support for adding a URI based boot
> option. This boot option can then be used for HTTP boot.

With your patch when I try to add a boot option I was surprised to see 
this menu:

   ** Add Boot Option **

       Description:
       File:
       Initrd File:
       Fdt File:
       Optional Data:
       Uri:
       Save
       Quit

My expectation is that I can define a URL each for

* binary
* initrd
* fdt

It should be possible to choose a network device in this menu instead:

   ** Select Volume **

       nvme 0:1
       nvme 0:2
       mmc 0:1
       mmc 0:2
       Quit

> 
> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> ---
> 
> Note: I had explored adding a corresponding test for this change. But
> looks like the eficonfig command itself is not being tested since
> commit 952018117ab4 ("dm: sandbox: Switch over to using the new host
> uclass"). This needs to be taken up separately as to why the eficonfig
> test has been bypassed, before adding stuff to the test.
> 
> 
>   cmd/eficonfig.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 87 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 6e14d34a6bd..f69902b76f7 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -35,6 +35,7 @@ static int avail_row;
>   
>   #define EFICONFIG_DESCRIPTION_MAX 32
>   #define EFICONFIG_OPTIONAL_DATA_MAX 64
> +#define EFICONFIG_URI_MAX 512
>   #define EFICONFIG_MENU_HEADER_ROW_NUM 3
>   #define EFICONFIG_MENU_DESC_ROW_NUM 5
>   
> @@ -57,6 +58,7 @@ struct eficonfig_filepath_info {
>    * @boot_index:		index of the boot option
>    * @description:	pointer to the description string
>    * @optional_data:	pointer to the optional_data
> + * @uri:		URI for HTTP Boot
>    * @edit_completed:	flag indicates edit complete
>    */
>   struct eficonfig_boot_option {
> @@ -66,6 +68,7 @@ struct eficonfig_boot_option {
>   	unsigned int boot_index;
>   	u16 *description;
>   	u16 *optional_data;
> +	u16 *uri;
>   	bool edit_completed;
>   };
>   
> @@ -538,6 +541,31 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
>   	return dp;
>   }
>   
> +static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str)
> +{
> +	char *pos, *p;
> +	u32 len = 0;
> +	efi_uintn_t uridp_len;
> +	struct efi_device_path_uri *uridp;
> +
> +	len = utf16_utf8_strlen(uri_str);

The code in this function seems not to guarantee valid URLs. E.g. you 
don't check that the schema is http:// or https://.

Please, provide unit tests for the URL validation.

The UEFI spec refers to RFC 3986 for the allowable content of a Uri() 
device-path.

See also https://url.spec.whatwg.org/#valid-url-string

The Chinese URL https://www.hk01.com/zone/1/港聞 should be encoded as 
https://www.hk01.com/zone/1/%E6%B8%AF%E8%81%9E.

The following steps are required:

1) convert to UTF-8
2) encode bytes in URL fragements using hex encoding as needed.

See https://url.spec.whatwg.org/#url-fragment-string

Please, implement unit tests for URL encoding.

Best regards

Heinrich

> +
> +	uridp_len = sizeof(struct efi_device_path) + len + 1;
> +	uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END));
> +	if (!uridp)
> +		return NULL;
> +
> +	uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> +	uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> +	uridp->dp.length = uridp_len;
> +	p = (char *)&uridp->uri;
> +	utf16_utf8_strcpy(&p, uri_str);
> +	pos = (char *)uridp + uridp_len;
> +	memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END));
> +
> +	return &uridp->dp;
> +}
> +
>   /**
>    * eficonfig_file_selected() - handler of file selection
>    *
> @@ -983,6 +1011,22 @@ static efi_status_t eficonfig_boot_add_optional_data(void *data)
>   				 "  enter optional data:");
>   }
>   
> +/**
> + * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI
> + *
> + * @data:	pointer to the internal boot option structure
> + * Return:	status code
> + */
> +static efi_status_t eficonfig_boot_add_uri(void *data)
> +{
> +	struct eficonfig_boot_option *bo = data;
> +
> +	return handle_user_input(bo->uri, EFICONFIG_URI_MAX, 24,
> +				 "\n  ** Edit URI **\n"
> +				 "\n"
> +				 "  enter HTTP Boot URI:");
> +}
> +
>   /**
>    * eficonfig_boot_edit_save() - handler to save the boot option
>    *
> @@ -998,7 +1042,8 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
>   		bo->edit_completed = false;
>   		return EFI_NOT_READY;
>   	}
> -	if (u16_strlen(bo->file_info.current_path) == 0) {
> +	if (u16_strlen(bo->file_info.current_path) == 0 &&
> +	    u16_strlen(bo->uri) == 0) {
>   		eficonfig_print_msg("File is not selected!");
>   		bo->edit_completed = false;
>   		return EFI_NOT_READY;
> @@ -1318,6 +1363,11 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
>   	if (ret != EFI_SUCCESS)
>   		goto out;
>   
> +	ret = create_boot_option_entry(efi_menu, "Uri: ", bo->uri,
> +				       eficonfig_boot_add_uri, bo);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
>   	ret = create_boot_option_entry(efi_menu, "Save", NULL,
>   				       eficonfig_boot_edit_save, bo);
>   	if (ret != EFI_SUCCESS)
> @@ -1340,6 +1390,22 @@ out:
>   	return ret;
>   }
>   
> +static bool eficonfig_fp_uri(struct efi_device_path *dp)
> +{
> +	return dp->type == DEVICE_PATH_TYPE_MESSAGING_DEVICE &&
> +		dp->sub_type == DEVICE_PATH_SUB_TYPE_MSG_URI;
> +}
> +
> +static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
> +{
> +	u16 *p = *uri_str;
> +	struct efi_device_path_uri *uridp;
> +
> +	uridp = (struct efi_device_path_uri *)dp;
> +
> +	utf8_utf16_strcpy(&p, uridp->uri);
> +}
> +
>   /**
>    * fill_file_info() - fill the file info from efi_device_path structure
>    *
> @@ -1392,10 +1458,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   	size_t len;
>   	efi_status_t ret;
>   	char *tmp = NULL, *p;
> +	u16 *current_path = NULL;
>   	struct efi_load_option lo = {0};
>   	efi_uintn_t dp_size;
>   	struct efi_device_path *dp = NULL;
>   	efi_uintn_t size = load_option_size;
> +	struct efi_device_path *dp_volume = NULL;
> +	struct efi_device_path *uri_dp = NULL;
>   	struct efi_device_path *device_dp = NULL;
>   	struct efi_device_path *initrd_dp = NULL;
>   	struct efi_device_path *fdt_dp = NULL;
> @@ -1464,6 +1533,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   		goto out;
>   	}
>   
> +	bo->uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
> +	if (!bo->uri) {
> +		ret =  EFI_OUT_OF_RESOURCES;
> +		goto out;
> +	}
> +
>   	/* copy the preset value */
>   	if (load_option) {
>   		ret = efi_deserialize_load_option(&lo, load_option, &size);
> @@ -1481,7 +1556,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   		u16_strcpy(bo->description, lo.label);
>   
>   		/* EFI image file path is a first instance */
> -		if (lo.file_path)
> +		if (lo.file_path && eficonfig_fp_uri(lo.file_path))
> +			fill_dp_uri(lo.file_path, &bo->uri);
> +		else
>   			fill_file_info(lo.file_path, &bo->file_info, device_dp);
>   
>   		/* Initrd file path (optional) is placed at second instance. */
> @@ -1512,6 +1589,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   			goto out;
>   	}
>   
> +	if (utf16_utf8_strlen(bo->uri))
> +		uri_dp = eficonfig_create_uri_device_path(bo->uri);
> +
>   	if (bo->initrd_info.dp_volume) {
>   		dp = eficonfig_create_device_path(bo->initrd_info.dp_volume,
>   						 bo->initrd_info.current_path);
> @@ -1536,7 +1616,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   		efi_free_pool(dp);
>   	}
>   
> -	dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
> +	dp_volume = bo->file_info.dp_volume;
> +	current_path = bo->file_info.current_path;
> +	dp = uri_dp ?
> +		uri_dp : eficonfig_create_device_path(dp_volume, current_path);
>   	if (!dp) {
>   		ret = EFI_OUT_OF_RESOURCES;
>   		goto out;
> @@ -1558,6 +1641,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
>   	ret = eficonfig_set_boot_option(varname, dp, dp_size, bo->description, tmp);
>   out:
>   	free(tmp);
> +	free(bo->uri);
>   	free(bo->optional_data);
>   	free(bo->description);
>   	free(bo->file_info.current_path);
Ilias Apalodimas June 17, 2025, 6:15 a.m. UTC | #2
Hi Heinrich, Sughosh

On Tue, 17 Jun 2025 at 00:53, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/16/25 12:23, Sughosh Ganu wrote:
> > The eficonfig command provides a menu based interface for maintenance
> > of the EFI boot options. Add support for adding a URI based boot
> > option. This boot option can then be used for HTTP boot.
>
> With your patch when I try to add a boot option I was surprised to see
> this menu:
>
>    ** Add Boot Option **
>
>        Description:
>        File:
>        Initrd File:
>        Fdt File:
>        Optional Data:
>        Uri:

I think this is going to confuse people. Can we just move it with
'File' and instead have 'File or URI"?

>        Save
>        Quit
>
> My expectation is that I can define a URL each for
>
> * binary
> * initrd
> * fdt

Hmm why? Perhaps the FDT makes some sense. But for distros currently
the initrd is supplied by GRUB. Even if we download our own, using it
is impossible at the moment. I think we should just add the URI for
the OS for now.

>
> It should be possible to choose a network device in this menu instead:
>
>    ** Select Volume **
>

Yes that would be a good idea. And I assume choosing it would
automatically set ethact as well?

>        nvme 0:1
>        nvme 0:2
>        mmc 0:1
>        mmc 0:2
>        Quit
>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Note: I had explored adding a corresponding test for this change. But
> > looks like the eficonfig command itself is not being tested since
> > commit 952018117ab4 ("dm: sandbox: Switch over to using the new host
> > uclass"). This needs to be taken up separately as to why the eficonfig
> > test has been bypassed, before adding stuff to the test.
> >
> >
> >   cmd/eficonfig.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 6e14d34a6bd..f69902b76f7 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -35,6 +35,7 @@ static int avail_row;
> >
> >   #define EFICONFIG_DESCRIPTION_MAX 32
> >   #define EFICONFIG_OPTIONAL_DATA_MAX 64
> > +#define EFICONFIG_URI_MAX 512
> >   #define EFICONFIG_MENU_HEADER_ROW_NUM 3
> >   #define EFICONFIG_MENU_DESC_ROW_NUM 5
> >
> > @@ -57,6 +58,7 @@ struct eficonfig_filepath_info {
> >    * @boot_index:             index of the boot option
> >    * @description:    pointer to the description string
> >    * @optional_data:  pointer to the optional_data
> > + * @uri:             URI for HTTP Boot
> >    * @edit_completed: flag indicates edit complete
> >    */
> >   struct eficonfig_boot_option {
> > @@ -66,6 +68,7 @@ struct eficonfig_boot_option {
> >       unsigned int boot_index;
> >       u16 *description;
> >       u16 *optional_data;
> > +     u16 *uri;
> >       bool edit_completed;
> >   };
> >
> > @@ -538,6 +541,31 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
> >       return dp;
> >   }
> >
> > +static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str)
> > +{
> > +     char *pos, *p;
> > +     u32 len = 0;
> > +     efi_uintn_t uridp_len;
> > +     struct efi_device_path_uri *uridp;
> > +
> > +     len = utf16_utf8_strlen(uri_str);
>
> The code in this function seems not to guarantee valid URLs. E.g. you
> don't check that the schema is http:// or https://.
>
> Please, provide unit tests for the URL validation.

We already have some of that in wget. Just export those functions and
re-use them

>
> The UEFI spec refers to RFC 3986 for the allowable content of a Uri()
> device-path.
>
> See also https://url.spec.whatwg.org/#valid-url-string
>
> The Chinese URL https://www.hk01.com/zone/1/港聞 should be encoded as
> https://www.hk01.com/zone/1/%E6%B8%AF%E8%81%9E.
>
> The following steps are required:
>
> 1) convert to UTF-8
> 2) encode bytes in URL fragements using hex encoding as needed.
>
> See https://url.spec.whatwg.org/#url-fragment-string
>
> Please, implement unit tests for URL encoding.
>
> Best regards
>
> Heinrich

Thnaks
/Ilias
>
> > +
> > +     uridp_len = sizeof(struct efi_device_path) + len + 1;
> > +     uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END));
> > +     if (!uridp)
> > +             return NULL;
> > +
> > +     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > +     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +     uridp->dp.length = uridp_len;
> > +     p = (char *)&uridp->uri;
> > +     utf16_utf8_strcpy(&p, uri_str);
> > +     pos = (char *)uridp + uridp_len;
> > +     memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END));
> > +
> > +     return &uridp->dp;
> > +}
> > +
> >   /**
> >    * eficonfig_file_selected() - handler of file selection
> >    *
> > @@ -983,6 +1011,22 @@ static efi_status_t eficonfig_boot_add_optional_data(void *data)
> >                                "  enter optional data:");
> >   }
> >
> > +/**
> > + * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI
> > + *
> > + * @data:    pointer to the internal boot option structure
> > + * Return:   status code
> > + */
> > +static efi_status_t eficonfig_boot_add_uri(void *data)
> > +{
> > +     struct eficonfig_boot_option *bo = data;
> > +
> > +     return handle_user_input(bo->uri, EFICONFIG_URI_MAX, 24,
> > +                              "\n  ** Edit URI **\n"
> > +                              "\n"
> > +                              "  enter HTTP Boot URI:");
> > +}
> > +
> >   /**
> >    * eficonfig_boot_edit_save() - handler to save the boot option
> >    *
> > @@ -998,7 +1042,8 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> >       }
> > -     if (u16_strlen(bo->file_info.current_path) == 0) {
> > +     if (u16_strlen(bo->file_info.current_path) == 0 &&
> > +         u16_strlen(bo->uri) == 0) {
> >               eficonfig_print_msg("File is not selected!");
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> > @@ -1318,6 +1363,11 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
> >       if (ret != EFI_SUCCESS)
> >               goto out;
> >
> > +     ret = create_boot_option_entry(efi_menu, "Uri: ", bo->uri,
> > +                                    eficonfig_boot_add_uri, bo);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> >       ret = create_boot_option_entry(efi_menu, "Save", NULL,
> >                                      eficonfig_boot_edit_save, bo);
> >       if (ret != EFI_SUCCESS)
> > @@ -1340,6 +1390,22 @@ out:
> >       return ret;
> >   }
> >
> > +static bool eficonfig_fp_uri(struct efi_device_path *dp)
> > +{
> > +     return dp->type == DEVICE_PATH_TYPE_MESSAGING_DEVICE &&
> > +             dp->sub_type == DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +}
> > +
> > +static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
> > +{
> > +     u16 *p = *uri_str;
> > +     struct efi_device_path_uri *uridp;
> > +
> > +     uridp = (struct efi_device_path_uri *)dp;
> > +
> > +     utf8_utf16_strcpy(&p, uridp->uri);
> > +}
> > +
> >   /**
> >    * fill_file_info() - fill the file info from efi_device_path structure
> >    *
> > @@ -1392,10 +1458,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >       size_t len;
> >       efi_status_t ret;
> >       char *tmp = NULL, *p;
> > +     u16 *current_path = NULL;
> >       struct efi_load_option lo = {0};
> >       efi_uintn_t dp_size;
> >       struct efi_device_path *dp = NULL;
> >       efi_uintn_t size = load_option_size;
> > +     struct efi_device_path *dp_volume = NULL;
> > +     struct efi_device_path *uri_dp = NULL;
> >       struct efi_device_path *device_dp = NULL;
> >       struct efi_device_path *initrd_dp = NULL;
> >       struct efi_device_path *fdt_dp = NULL;
> > @@ -1464,6 +1533,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               goto out;
> >       }
> >
> > +     bo->uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
> > +     if (!bo->uri) {
> > +             ret =  EFI_OUT_OF_RESOURCES;
> > +             goto out;
> > +     }
> > +
> >       /* copy the preset value */
> >       if (load_option) {
> >               ret = efi_deserialize_load_option(&lo, load_option, &size);
> > @@ -1481,7 +1556,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               u16_strcpy(bo->description, lo.label);
> >
> >               /* EFI image file path is a first instance */
> > -             if (lo.file_path)
> > +             if (lo.file_path && eficonfig_fp_uri(lo.file_path))
> > +                     fill_dp_uri(lo.file_path, &bo->uri);
> > +             else
> >                       fill_file_info(lo.file_path, &bo->file_info, device_dp);
> >
> >               /* Initrd file path (optional) is placed at second instance. */
> > @@ -1512,6 +1589,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >                       goto out;
> >       }
> >
> > +     if (utf16_utf8_strlen(bo->uri))
> > +             uri_dp = eficonfig_create_uri_device_path(bo->uri);
> > +
> >       if (bo->initrd_info.dp_volume) {
> >               dp = eficonfig_create_device_path(bo->initrd_info.dp_volume,
> >                                                bo->initrd_info.current_path);
> > @@ -1536,7 +1616,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               efi_free_pool(dp);
> >       }
> >
> > -     dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
> > +     dp_volume = bo->file_info.dp_volume;
> > +     current_path = bo->file_info.current_path;
> > +     dp = uri_dp ?
> > +             uri_dp : eficonfig_create_device_path(dp_volume, current_path);
> >       if (!dp) {
> >               ret = EFI_OUT_OF_RESOURCES;
> >               goto out;
> > @@ -1558,6 +1641,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >       ret = eficonfig_set_boot_option(varname, dp, dp_size, bo->description, tmp);
> >   out:
> >       free(tmp);
> > +     free(bo->uri);
> >       free(bo->optional_data);
> >       free(bo->description);
> >       free(bo->file_info.current_path);
>
Sughosh Ganu June 17, 2025, 6:15 a.m. UTC | #3
On Tue, 17 Jun 2025 at 03:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 6/16/25 12:23, Sughosh Ganu wrote:
> > The eficonfig command provides a menu based interface for maintenance
> > of the EFI boot options. Add support for adding a URI based boot
> > option. This boot option can then be used for HTTP boot.
>
> With your patch when I try to add a boot option I was surprised to see
> this menu:
>
>    ** Add Boot Option **
>
>        Description:
>        File:
>        Initrd File:
>        Fdt File:
>        Optional Data:
>        Uri:
>        Save
>        Quit
>
> My expectation is that I can define a URL each for
>
> * binary
> * initrd
> * fdt
>
> It should be possible to choose a network device in this menu instead:
>
>    ** Select Volume **
>
>        nvme 0:1
>        nvme 0:2
>        mmc 0:1
>        mmc 0:2
>        Quit

This is an interesting suggestion. But this patch is bringing the
eficonfig command in line with the other command that is being used
for setting the boot options, namely efidebug. Moreover, if there is a
plan and interest to support getting the initrd and fdt files over the
network through an URI based device-path, we first need to add support
for the same in the EFI bootmgr. Then it would make sense to change
both the commands (eficonfig and efidebug) accordingly.

>
> >
> > Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
> > ---
> >
> > Note: I had explored adding a corresponding test for this change. But
> > looks like the eficonfig command itself is not being tested since
> > commit 952018117ab4 ("dm: sandbox: Switch over to using the new host
> > uclass"). This needs to be taken up separately as to why the eficonfig
> > test has been bypassed, before adding stuff to the test.
> >
> >
> >   cmd/eficonfig.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 87 insertions(+), 3 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 6e14d34a6bd..f69902b76f7 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -35,6 +35,7 @@ static int avail_row;
> >
> >   #define EFICONFIG_DESCRIPTION_MAX 32
> >   #define EFICONFIG_OPTIONAL_DATA_MAX 64
> > +#define EFICONFIG_URI_MAX 512
> >   #define EFICONFIG_MENU_HEADER_ROW_NUM 3
> >   #define EFICONFIG_MENU_DESC_ROW_NUM 5
> >
> > @@ -57,6 +58,7 @@ struct eficonfig_filepath_info {
> >    * @boot_index:             index of the boot option
> >    * @description:    pointer to the description string
> >    * @optional_data:  pointer to the optional_data
> > + * @uri:             URI for HTTP Boot
> >    * @edit_completed: flag indicates edit complete
> >    */
> >   struct eficonfig_boot_option {
> > @@ -66,6 +68,7 @@ struct eficonfig_boot_option {
> >       unsigned int boot_index;
> >       u16 *description;
> >       u16 *optional_data;
> > +     u16 *uri;
> >       bool edit_completed;
> >   };
> >
> > @@ -538,6 +541,31 @@ struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
> >       return dp;
> >   }
> >
> > +static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str)
> > +{
> > +     char *pos, *p;
> > +     u32 len = 0;
> > +     efi_uintn_t uridp_len;
> > +     struct efi_device_path_uri *uridp;
> > +
> > +     len = utf16_utf8_strlen(uri_str);
>
> The code in this function seems not to guarantee valid URLs. E.g. you
> don't check that the schema is http:// or https://.
>
> Please, provide unit tests for the URL validation.
>
> The UEFI spec refers to RFC 3986 for the allowable content of a Uri()
> device-path.
>
> See also https://url.spec.whatwg.org/#valid-url-string
>
> The Chinese URL https://www.hk01.com/zone/1/港聞 should be encoded as
> https://www.hk01.com/zone/1/%E6%B8%AF%E8%81%9E.
>
> The following steps are required:
>
> 1) convert to UTF-8
> 2) encode bytes in URL fragements using hex encoding as needed.

Fwiw, I did consider adding a check for the URI, but then I saw that
the lwip stack does this in parse_url() through a call to
wget_validate_uri().

>
> See https://url.spec.whatwg.org/#url-fragment-string
>
> Please, implement unit tests for URL encoding.

Okay, but as I mentioned in the patch note, the entire eficonfig test
has been bypassed for some unknown reason. This too needs
investigation.

-sughosh

>
> Best regards
>
> Heinrich
>
> > +
> > +     uridp_len = sizeof(struct efi_device_path) + len + 1;
> > +     uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END));
> > +     if (!uridp)
> > +             return NULL;
> > +
> > +     uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
> > +     uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +     uridp->dp.length = uridp_len;
> > +     p = (char *)&uridp->uri;
> > +     utf16_utf8_strcpy(&p, uri_str);
> > +     pos = (char *)uridp + uridp_len;
> > +     memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END));
> > +
> > +     return &uridp->dp;
> > +}
> > +
> >   /**
> >    * eficonfig_file_selected() - handler of file selection
> >    *
> > @@ -983,6 +1011,22 @@ static efi_status_t eficonfig_boot_add_optional_data(void *data)
> >                                "  enter optional data:");
> >   }
> >
> > +/**
> > + * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI
> > + *
> > + * @data:    pointer to the internal boot option structure
> > + * Return:   status code
> > + */
> > +static efi_status_t eficonfig_boot_add_uri(void *data)
> > +{
> > +     struct eficonfig_boot_option *bo = data;
> > +
> > +     return handle_user_input(bo->uri, EFICONFIG_URI_MAX, 24,
> > +                              "\n  ** Edit URI **\n"
> > +                              "\n"
> > +                              "  enter HTTP Boot URI:");
> > +}
> > +
> >   /**
> >    * eficonfig_boot_edit_save() - handler to save the boot option
> >    *
> > @@ -998,7 +1042,8 @@ static efi_status_t eficonfig_boot_edit_save(void *data)
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> >       }
> > -     if (u16_strlen(bo->file_info.current_path) == 0) {
> > +     if (u16_strlen(bo->file_info.current_path) == 0 &&
> > +         u16_strlen(bo->uri) == 0) {
> >               eficonfig_print_msg("File is not selected!");
> >               bo->edit_completed = false;
> >               return EFI_NOT_READY;
> > @@ -1318,6 +1363,11 @@ static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
> >       if (ret != EFI_SUCCESS)
> >               goto out;
> >
> > +     ret = create_boot_option_entry(efi_menu, "Uri: ", bo->uri,
> > +                                    eficonfig_boot_add_uri, bo);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> >       ret = create_boot_option_entry(efi_menu, "Save", NULL,
> >                                      eficonfig_boot_edit_save, bo);
> >       if (ret != EFI_SUCCESS)
> > @@ -1340,6 +1390,22 @@ out:
> >       return ret;
> >   }
> >
> > +static bool eficonfig_fp_uri(struct efi_device_path *dp)
> > +{
> > +     return dp->type == DEVICE_PATH_TYPE_MESSAGING_DEVICE &&
> > +             dp->sub_type == DEVICE_PATH_SUB_TYPE_MSG_URI;
> > +}
> > +
> > +static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
> > +{
> > +     u16 *p = *uri_str;
> > +     struct efi_device_path_uri *uridp;
> > +
> > +     uridp = (struct efi_device_path_uri *)dp;
> > +
> > +     utf8_utf16_strcpy(&p, uridp->uri);
> > +}
> > +
> >   /**
> >    * fill_file_info() - fill the file info from efi_device_path structure
> >    *
> > @@ -1392,10 +1458,13 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >       size_t len;
> >       efi_status_t ret;
> >       char *tmp = NULL, *p;
> > +     u16 *current_path = NULL;
> >       struct efi_load_option lo = {0};
> >       efi_uintn_t dp_size;
> >       struct efi_device_path *dp = NULL;
> >       efi_uintn_t size = load_option_size;
> > +     struct efi_device_path *dp_volume = NULL;
> > +     struct efi_device_path *uri_dp = NULL;
> >       struct efi_device_path *device_dp = NULL;
> >       struct efi_device_path *initrd_dp = NULL;
> >       struct efi_device_path *fdt_dp = NULL;
> > @@ -1464,6 +1533,12 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               goto out;
> >       }
> >
> > +     bo->uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
> > +     if (!bo->uri) {
> > +             ret =  EFI_OUT_OF_RESOURCES;
> > +             goto out;
> > +     }
> > +
> >       /* copy the preset value */
> >       if (load_option) {
> >               ret = efi_deserialize_load_option(&lo, load_option, &size);
> > @@ -1481,7 +1556,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               u16_strcpy(bo->description, lo.label);
> >
> >               /* EFI image file path is a first instance */
> > -             if (lo.file_path)
> > +             if (lo.file_path && eficonfig_fp_uri(lo.file_path))
> > +                     fill_dp_uri(lo.file_path, &bo->uri);
> > +             else
> >                       fill_file_info(lo.file_path, &bo->file_info, device_dp);
> >
> >               /* Initrd file path (optional) is placed at second instance. */
> > @@ -1512,6 +1589,9 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >                       goto out;
> >       }
> >
> > +     if (utf16_utf8_strlen(bo->uri))
> > +             uri_dp = eficonfig_create_uri_device_path(bo->uri);
> > +
> >       if (bo->initrd_info.dp_volume) {
> >               dp = eficonfig_create_device_path(bo->initrd_info.dp_volume,
> >                                                bo->initrd_info.current_path);
> > @@ -1536,7 +1616,10 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >               efi_free_pool(dp);
> >       }
> >
> > -     dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
> > +     dp_volume = bo->file_info.dp_volume;
> > +     current_path = bo->file_info.current_path;
> > +     dp = uri_dp ?
> > +             uri_dp : eficonfig_create_device_path(dp_volume, current_path);
> >       if (!dp) {
> >               ret = EFI_OUT_OF_RESOURCES;
> >               goto out;
> > @@ -1558,6 +1641,7 @@ static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
> >       ret = eficonfig_set_boot_option(varname, dp, dp_size, bo->description, tmp);
> >   out:
> >       free(tmp);
> > +     free(bo->uri);
> >       free(bo->optional_data);
> >       free(bo->description);
> >       free(bo->file_info.current_path);
>
diff mbox series

Patch

diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
index 6e14d34a6bd..f69902b76f7 100644
--- a/cmd/eficonfig.c
+++ b/cmd/eficonfig.c
@@ -35,6 +35,7 @@  static int avail_row;
 
 #define EFICONFIG_DESCRIPTION_MAX 32
 #define EFICONFIG_OPTIONAL_DATA_MAX 64
+#define EFICONFIG_URI_MAX 512
 #define EFICONFIG_MENU_HEADER_ROW_NUM 3
 #define EFICONFIG_MENU_DESC_ROW_NUM 5
 
@@ -57,6 +58,7 @@  struct eficonfig_filepath_info {
  * @boot_index:		index of the boot option
  * @description:	pointer to the description string
  * @optional_data:	pointer to the optional_data
+ * @uri:		URI for HTTP Boot
  * @edit_completed:	flag indicates edit complete
  */
 struct eficonfig_boot_option {
@@ -66,6 +68,7 @@  struct eficonfig_boot_option {
 	unsigned int boot_index;
 	u16 *description;
 	u16 *optional_data;
+	u16 *uri;
 	bool edit_completed;
 };
 
@@ -538,6 +541,31 @@  struct efi_device_path *eficonfig_create_device_path(struct efi_device_path *dp_
 	return dp;
 }
 
+static struct efi_device_path *eficonfig_create_uri_device_path(u16 *uri_str)
+{
+	char *pos, *p;
+	u32 len = 0;
+	efi_uintn_t uridp_len;
+	struct efi_device_path_uri *uridp;
+
+	len = utf16_utf8_strlen(uri_str);
+
+	uridp_len = sizeof(struct efi_device_path) + len + 1;
+	uridp = efi_alloc(uridp_len + sizeof(EFI_DP_END));
+	if (!uridp)
+		return NULL;
+
+	uridp->dp.type = DEVICE_PATH_TYPE_MESSAGING_DEVICE;
+	uridp->dp.sub_type = DEVICE_PATH_SUB_TYPE_MSG_URI;
+	uridp->dp.length = uridp_len;
+	p = (char *)&uridp->uri;
+	utf16_utf8_strcpy(&p, uri_str);
+	pos = (char *)uridp + uridp_len;
+	memcpy(pos, &EFI_DP_END, sizeof(EFI_DP_END));
+
+	return &uridp->dp;
+}
+
 /**
  * eficonfig_file_selected() - handler of file selection
  *
@@ -983,6 +1011,22 @@  static efi_status_t eficonfig_boot_add_optional_data(void *data)
 				 "  enter optional data:");
 }
 
+/**
+ * eficonfig_boot_add_uri() - handle user input for HTTP Boot URI
+ *
+ * @data:	pointer to the internal boot option structure
+ * Return:	status code
+ */
+static efi_status_t eficonfig_boot_add_uri(void *data)
+{
+	struct eficonfig_boot_option *bo = data;
+
+	return handle_user_input(bo->uri, EFICONFIG_URI_MAX, 24,
+				 "\n  ** Edit URI **\n"
+				 "\n"
+				 "  enter HTTP Boot URI:");
+}
+
 /**
  * eficonfig_boot_edit_save() - handler to save the boot option
  *
@@ -998,7 +1042,8 @@  static efi_status_t eficonfig_boot_edit_save(void *data)
 		bo->edit_completed = false;
 		return EFI_NOT_READY;
 	}
-	if (u16_strlen(bo->file_info.current_path) == 0) {
+	if (u16_strlen(bo->file_info.current_path) == 0 &&
+	    u16_strlen(bo->uri) == 0) {
 		eficonfig_print_msg("File is not selected!");
 		bo->edit_completed = false;
 		return EFI_NOT_READY;
@@ -1318,6 +1363,11 @@  static efi_status_t eficonfig_show_boot_option(struct eficonfig_boot_option *bo,
 	if (ret != EFI_SUCCESS)
 		goto out;
 
+	ret = create_boot_option_entry(efi_menu, "Uri: ", bo->uri,
+				       eficonfig_boot_add_uri, bo);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
 	ret = create_boot_option_entry(efi_menu, "Save", NULL,
 				       eficonfig_boot_edit_save, bo);
 	if (ret != EFI_SUCCESS)
@@ -1340,6 +1390,22 @@  out:
 	return ret;
 }
 
+static bool eficonfig_fp_uri(struct efi_device_path *dp)
+{
+	return dp->type == DEVICE_PATH_TYPE_MESSAGING_DEVICE &&
+		dp->sub_type == DEVICE_PATH_SUB_TYPE_MSG_URI;
+}
+
+static void fill_dp_uri(struct efi_device_path *dp, u16 **uri_str)
+{
+	u16 *p = *uri_str;
+	struct efi_device_path_uri *uridp;
+
+	uridp = (struct efi_device_path_uri *)dp;
+
+	utf8_utf16_strcpy(&p, uridp->uri);
+}
+
 /**
  * fill_file_info() - fill the file info from efi_device_path structure
  *
@@ -1392,10 +1458,13 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 	size_t len;
 	efi_status_t ret;
 	char *tmp = NULL, *p;
+	u16 *current_path = NULL;
 	struct efi_load_option lo = {0};
 	efi_uintn_t dp_size;
 	struct efi_device_path *dp = NULL;
 	efi_uintn_t size = load_option_size;
+	struct efi_device_path *dp_volume = NULL;
+	struct efi_device_path *uri_dp = NULL;
 	struct efi_device_path *device_dp = NULL;
 	struct efi_device_path *initrd_dp = NULL;
 	struct efi_device_path *fdt_dp = NULL;
@@ -1464,6 +1533,12 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 		goto out;
 	}
 
+	bo->uri = calloc(1, EFICONFIG_URI_MAX * sizeof(u16));
+	if (!bo->uri) {
+		ret =  EFI_OUT_OF_RESOURCES;
+		goto out;
+	}
+
 	/* copy the preset value */
 	if (load_option) {
 		ret = efi_deserialize_load_option(&lo, load_option, &size);
@@ -1481,7 +1556,9 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 		u16_strcpy(bo->description, lo.label);
 
 		/* EFI image file path is a first instance */
-		if (lo.file_path)
+		if (lo.file_path && eficonfig_fp_uri(lo.file_path))
+			fill_dp_uri(lo.file_path, &bo->uri);
+		else
 			fill_file_info(lo.file_path, &bo->file_info, device_dp);
 
 		/* Initrd file path (optional) is placed at second instance. */
@@ -1512,6 +1589,9 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 			goto out;
 	}
 
+	if (utf16_utf8_strlen(bo->uri))
+		uri_dp = eficonfig_create_uri_device_path(bo->uri);
+
 	if (bo->initrd_info.dp_volume) {
 		dp = eficonfig_create_device_path(bo->initrd_info.dp_volume,
 						 bo->initrd_info.current_path);
@@ -1536,7 +1616,10 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 		efi_free_pool(dp);
 	}
 
-	dp = eficonfig_create_device_path(bo->file_info.dp_volume, bo->file_info.current_path);
+	dp_volume = bo->file_info.dp_volume;
+	current_path = bo->file_info.current_path;
+	dp = uri_dp ?
+		uri_dp : eficonfig_create_device_path(dp_volume, current_path);
 	if (!dp) {
 		ret = EFI_OUT_OF_RESOURCES;
 		goto out;
@@ -1558,6 +1641,7 @@  static efi_status_t eficonfig_edit_boot_option(u16 *varname, struct eficonfig_bo
 	ret = eficonfig_set_boot_option(varname, dp, dp_size, bo->description, tmp);
 out:
 	free(tmp);
+	free(bo->uri);
 	free(bo->optional_data);
 	free(bo->description);
 	free(bo->file_info.current_path);