Re: [PATCH v2] efi_loader: fix use of uninitialized guid in variable enumeration loops

On 7/28/26 21:33, Scott Moser wrote:
> efi_bootmgr_delete_invalid_boot_option(), eficonfig_show_boot_selection(),
> and eficonfig_create_change_boot_order_entry() each enumerate all EFI
> variables by repeatedly calling efi_next_variable_name() in a loop,
> passing the same efi_guid_t as both input and output. GetNextVariableName()
> needs the vendor GUID returned by the previous call, together with the
> variable name it returned, to know where to resume.
> 
> In each of these loops the efi_guid_t was declared inside the loop body,
> so a new instance comes into scope on every iteration. Relying on it to
> still hold the previous iteration's value depends on the compiler reusing
> the same stack slot across iterations, which is undefined behavior. With
> a compiler that zero-initializes locals by default (e.g. clang, or gcc
> configured with -ftrivial-auto-var-init=zero), the GUID is cleared on
> every iteration, so the lookup of the variable name returned by the
> previous call fails and efi_init_obj_list() aborts:
> 
>    Cannot initialize UEFI sub-system
>    ** Booting bootflow ... with efi
>    Boot failed (err=-22)
> 
> Move the efi_guid_t declarations out of the loops so the value written
> by the previous efi_next_variable_name() call is preserved across
> iterations.
> 
> Fixes: 140a8959d48f ("eficonfig: use efi_get_next_variable_name_int()")
> Signed-off-by: Scott Moser <[email protected]>

Reviewed-by: Heinrich Schuchardt <[email protected]>

None of the loops handles EFI_BUFFER_TOO_SMALL. Just create a variable 
called fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210 
to break the code.

We should move all calls of GetNextVariable() to a new function in 
lib/efi_loader/efi_var_common.c with proper handling of the variable 
name buffer.

But let's merge the current patch first.

Best regards

Heinrich

> ---
> Changes for v2:
> - Drop the "= {}" initializer on the hoisted guid: per the UEFI spec,
>    VendorGuid is ignored when VariableName is an empty string, which is
>    the state var_name16 is in on the first call, so the initial value is
>    never read and zero-initializing it only costs code size.
> 
>   cmd/eficonfig.c              | 4 ++--
>   lib/efi_loader/efi_bootmgr.c | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c
> index 4d060e3007c..d8e7ed6666a 100644
> --- a/cmd/eficonfig.c
> +++ b/cmd/eficonfig.c
> @@ -1844,6 +1844,7 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>   	struct efimenu *efi_menu;
>   	struct list_head *pos, *n;
>   	struct eficonfig_entry *entry;
> +	efi_guid_t guid;
>   
>   	efi_menu = calloc(1, sizeof(struct efimenu));
>   	if (!efi_menu)
> @@ -1872,7 +1873,6 @@ static efi_status_t eficonfig_show_boot_selection(unsigned int *selected)
>   	var_name16[0] = 0;
>   	for (;;) {
>   		int index;
> -		efi_guid_t guid;
>   
>   		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);
>   		if (ret == EFI_NOT_FOUND)
> @@ -2245,6 +2245,7 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>   	u16 *var_name16 = NULL;
>   	efi_uintn_t size, buf_size;
>   	struct eficonfig_save_boot_order_data *save_data;
> +	efi_guid_t guid;
>   
>   	/* list the load option in the order of BootOrder variable */
>   	for (i = 0; i < num; i++) {
> @@ -2265,7 +2266,6 @@ static efi_status_t eficonfig_create_change_boot_order_entry(struct efimenu *efi
>   	var_name16[0] = 0;
>   	for (;;) {
>   		int index;
> -		efi_guid_t guid;
>   
>   		if (efi_menu->count >= EFICONFIG_ENTRY_NUM_MAX - 2)
>   			break;
> diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c
> index 8c9a9b5eb56..8938b214ceb 100644
> --- a/lib/efi_loader/efi_bootmgr.c
> +++ b/lib/efi_loader/efi_bootmgr.c
> @@ -934,6 +934,7 @@ static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct eficonfig_medi
>   	efi_status_t ret = EFI_SUCCESS;
>   	u16 *delete_index_list = NULL, *p;
>   	efi_uintn_t buf_size;
> +	efi_guid_t guid;
>   
>   	buf_size = 128;
>   	var_name16 = malloc(buf_size);
> @@ -943,7 +944,6 @@ static efi_status_t efi_bootmgr_delete_invalid_boot_option(struct eficonfig_medi
>   	var_name16[0] = 0;
>   	for (;;) {
>   		int index;
> -		efi_guid_t guid;
>   		efi_uintn_t tmp;
>   
>   		ret = efi_next_variable_name(&buf_size, &var_name16, &guid);