[PATCH 1/2] eficonfig: curve out efi_get_next_variable_name_int calls
Masahisa Kojima
masahisa.kojima at linaro.org
Fri Dec 16 07:22:57 CET 2022
On Fri, 16 Dec 2022 at 10:51, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> %s/curve/carve/
Thank you for pointing out the typo.
>
> On 12/8/22 05:40, Masahisa Kojima wrote:
> > To retrieve the EFI variable name by efi_get_next_variable_name_int(),
> > the sequence of alloc -> efi_get_next_variable_name_int ->
> > realloc -> efi_get_next_variable_name_int is required.
> > In current code, this sequence repeatedly appears in
> > the several functions. It should be curved out a common function.
> >
> > This commit also fixes the missing free() of var_name16
> > in eficonfig_delete_invalid_boot_option().
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima at linaro.org>
> > ---
> > cmd/eficonfig.c | 62 +++++++++----------------------------
> > include/efi_loader.h | 2 ++
> > lib/efi_loader/efi_helper.c | 34 ++++++++++++++++++++
> > 3 files changed, 51 insertions(+), 47 deletions(-)
> >
> > diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> > index 394ae67cce..cd7a51cc7e 100644
> > --- a/cmd/eficonfig.c
> > +++ b/cmd/eficonfig.c
> > @@ -1683,7 +1683,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
> > u32 i;
> > u16 *bootorder;
> > efi_status_t ret;
> > - u16 *var_name16 = NULL, *p;
> > + u16 *var_name16 = NULL;
> > efi_uintn_t num, size, buf_size;
>
> buf_size = 0;
> see below
>
> > struct efimenu *efi_menu;
> > struct list_head *pos, *n;
> > @@ -1718,24 +1718,12 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
> > int index;
> > efi_guid_t guid;
> >
> > - size = buf_size;
> > - ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> > + ret = efi_get_variable_name(&buf_size, &var_name16, &guid);
> > if (ret == EFI_NOT_FOUND)
> > break;
> > - if (ret == EFI_BUFFER_TOO_SMALL) {
> > - buf_size = size;
> > - p = realloc(var_name16, buf_size);
> > - if (!p) {
> > - free(var_name16);
> > - return EFI_OUT_OF_RESOURCES;
> > - }
> > - var_name16 = p;
> > - ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> > - }
> > - if (ret != EFI_SUCCESS) {
> > - free(var_name16);
> > - return ret;
> > - }
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > if (efi_varname_is_load_option(var_name16, &index)) {
> > /* If the index is included in the BootOrder, skip it */
> > if (search_bootorder(bootorder, num, index, NULL))
> > @@ -2026,7 +2014,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
> > u32 i;
> > char *title;
> > efi_status_t ret;
> > - u16 *var_name16 = NULL, *p;
> > + u16 *var_name16 = NULL;
> > efi_uintn_t size, buf_size;
> >
> > /* list the load option in the order of BootOrder variable */
> > @@ -2054,19 +2042,9 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
> > break;
> >
> > size = buf_size;
> > - ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> > + ret = efi_get_variable_name(&buf_size, &var_name16, &guid);
> > if (ret == EFI_NOT_FOUND)
> > break;
> > - if (ret == EFI_BUFFER_TOO_SMALL) {
> > - buf_size = size;
> > - p = realloc(var_name16, buf_size);
> > - if (!p) {
> > - ret = EFI_OUT_OF_RESOURCES;
> > - goto out;
> > - }
> > - var_name16 = p;
> > - ret = efi_get_next_variable_name_int(&size, var_name16, &guid);
> > - }
> > if (ret != EFI_SUCCESS)
> > goto out;
> >
> > @@ -2336,10 +2314,10 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
> > efi_uintn_t size;
> > void *load_option;
> > struct efi_load_option lo;
> > - u16 *var_name16 = NULL, *p;
> > + u16 *var_name16 = NULL;
> > u16 varname[] = u"Boot####";
> > efi_status_t ret = EFI_SUCCESS;
> > - efi_uintn_t varname_size, buf_size;
> > + efi_uintn_t buf_size;
> >
> > buf_size = 128;
> > var_name16 = malloc(buf_size);
>
> Why allocate anything here? We already have a realloc() in
> efi_get_variable_name() which will do the job. Just start with
> var_name16 = NULL, buf_size = 0.
To start the EFI variable search with GetNextVariableName(), the first call of
efi_get_next_variable_name_int() requires a pointer to the
Null-terminated string
and non-zero buffer size.
So var_name16 = NULL, buf_size = 0 setup will not work as expected,
efi_get_next_variable_name_int() returns EFI_INVALID_PARAMETER in this case.
>
> > @@ -2352,24 +2330,12 @@ efi_status_t eficonfig_delete_invalid_boot_option(struct eficonfig_media_boot_op
> > efi_guid_t guid;
> > efi_uintn_t tmp;
> >
> > - varname_size = buf_size;
> > - ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
> > + ret = efi_get_variable_name(&buf_size, &var_name16, &guid);
> > if (ret == EFI_NOT_FOUND)
> > break;
> > - if (ret == EFI_BUFFER_TOO_SMALL) {
> > - buf_size = varname_size;
> > - p = realloc(var_name16, buf_size);
> > - if (!p) {
> > - free(var_name16);
> > - return EFI_OUT_OF_RESOURCES;
> > - }
> > - var_name16 = p;
> > - ret = efi_get_next_variable_name_int(&varname_size, var_name16, &guid);
> > - }
> > - if (ret != EFI_SUCCESS) {
> > - free(var_name16);
> > - return ret;
> > - }
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > if (!efi_varname_is_load_option(var_name16, &index))
> > continue;
> >
> > @@ -2407,6 +2373,8 @@ next:
> > }
> >
> > out:
> > + free(var_name16);
> > +
> > return ret;
> > }
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 0899e293e5..f80a16108a 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -708,6 +708,8 @@ int algo_to_len(const char *algo);
> > int efi_link_dev(efi_handle_t handle, struct udevice *dev);
> > int efi_unlink_dev(efi_handle_t handle);
> > bool efi_varname_is_load_option(u16 *var_name16, int *index);
> > +efi_status_t efi_get_variable_name(efi_uintn_t *size, u16 **buf,
> > + efi_guid_t *guid);
> >
> > /**
> > * efi_size_in_pages() - convert size in bytes to size in pages
> > diff --git a/lib/efi_loader/efi_helper.c b/lib/efi_loader/efi_helper.c
> > index 788cb9faec..ca9854ec79 100644
> > --- a/lib/efi_loader/efi_helper.c
> > +++ b/lib/efi_loader/efi_helper.c
> > @@ -223,3 +223,37 @@ bool efi_varname_is_load_option(u16 *var_name16, int *index)
> >
> > return false;
> > }
> > +
> > +/**
> > + * efi_get_variable_name_() - get variable name
> > + *
> > + * This function is a wrapper of efi_get_next_variable_name_int().
> > + * If efi_get_next_variable_name_int() returns EFI_BUFFER_TOO_SMALL,
> > + * @size and @buf are updated by new buffer size and realloced buffer.
> > + *
> > + * @size: pointer to the buffer size
> > + * @buf: pointer to the buffer
> > + * @guid: pointer to the guid
> > + * Return: status code
> > + */
> > +efi_status_t efi_get_variable_name(efi_uintn_t *size, u16 **buf, efi_guid_t *guid)
>
> We should have next in the function name, e.g. "efi_next_variable_name".
Yes, I agree.
>
> > +{
> > + u16 *p;
> > + efi_status_t ret;
> > + efi_uintn_t buf_size = *size;
> > +
> > + ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> > + if (ret == EFI_NOT_FOUND)
> > + return ret;
> > + if (ret == EFI_BUFFER_TOO_SMALL) {
> > + p = realloc(*buf, buf_size);
> > + if (!p)
>
> *buf should be freed here and set to NULL.
var_name16 should be allocated by a caller as described above,
let me keep the current code.
Thanks,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> > + return EFI_OUT_OF_RESOURCES;
> > +
> > + *buf = p;
> > + *size = buf_size;
> > + ret = efi_get_next_variable_name_int(&buf_size, *buf, guid);
> > + }
> > +
> > + return ret;
> > +}
>
More information about the U-Boot
mailing list