[PATCH] efi_loader: Fix UEFI variable error handling

[Heinrich Schuchardt]

On 11/9/23 04:55, Weizhao Ouyang wrote:
> Correct some UEFI variable error handing code paths.
>
> Signed-off-by: Weizhao Ouyang <o451686892 at gmail.com>
> ---
>   lib/efi_loader/efi_var_file.c | 1 +
>   lib/efi_loader/efi_variable.c | 8 ++++----
>   2 files changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index 62e071bd83..dbb9b1f3fc 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -236,6 +236,7 @@ efi_status_t efi_var_from_file(void)
>   		log_err("Invalid EFI variables file\n");
>   error:
>   	free(buf);
> +	return ret;

Hello Weizhao,

thank you for looking into the error handling.

U-Boot's UEFI variables can either be stored in file ubootefi.var in the
ESP or in the RPMB (Replay Protected Memory Block) partition of the
eMMC. The suggested changes are about handling of errors when reading or
writing the file ubootefi.var. Security relevant variables (PK, KEK, db,
dbx) are never read from file.

On first boot a file ubootefi.var will not exist. It has to be created
by U-Boot. If efi_var_from_file() would return an error if the file does
not exist or is corrupted, we would never be able to boot such a system.
This is why deliberately we return EFI_SUCCESS here. What is missing in
the code is a comment explaining the design decision.

>   #endif
>   	return EFI_SUCCESS;
>   }
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index be95ed44e6..13966297c6 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -350,17 +350,17 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
>
>   	if (var_type == EFI_AUTH_VAR_PK)
>   		ret = efi_init_secure_state();
> -	else
> -		ret = EFI_SUCCESS;

The two lines are unreachable code and should be removed.

> +	if (ret != EFI_SUCCESS)
> +		return ret;

These new lines should only be reached if var_type == EFI_AUTH_VAR_PK.

I am not sure what Would be the right error handling if
efi_init_secure_state() fails:

* Do we have to set PK to the old value?
* Should we still persist PK to ubootefi.var?

However we decide we should describe our decisions in a code comment.

>
>   	/*
>   	 * Write non-volatile EFI variables to file
>   	 * TODO: check if a value change has occured to avoid superfluous writes
>   	 */
>   	if (attributes & EFI_VARIABLE_NON_VOLATILE)
> -		efi_var_to_file();
> +		ret = efi_var_to_file();

The discussion here should focus on the treatment of errors in the
file-system.

The following error cases come to my mind:

* ESP partition is missing
* ESP FAT file system is corrupted
* There is no space on the ESP.
* The medium (e.g. SD card) is in write only mode

The current code gives priority to enable booting in all adverse
situations. Do you think this is a bad choice?

@Ilias: Please, chime in.

Best regards

Heinrich

>
> -	return EFI_SUCCESS;
> +	return ret;
>   }
>
>   efi_status_t efi_query_variable_info_int(u32 attributes,