Message ID | 20221208134022.28621-2-masahisa.kojima@linaro.org |
---|---|
State | Superseded |
Headers | show |
Series | fix eficonfig GetNextVariableName calls handling | expand |
%s/curve/carve/ 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@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. > @@ -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". > +{ > + 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. 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; > +}
On Fri, 16 Dec 2022 at 10:51, Heinrich Schuchardt <xypron.glpk@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@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; > > +} >
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; 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); @@ -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) +{ + 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) + return EFI_OUT_OF_RESOURCES; + + *buf = p; + *size = buf_size; + ret = efi_get_next_variable_name_int(&buf_size, *buf, guid); + } + + return ret; +}
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@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(-)