[PATCH] efi_loader: Fix UEFI variable error handling

[Weizhao Ouyang]

On Thu, Nov 9, 2023 at 9:57 PM Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> 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.
>

Hi Heinrich,

Yes, my intention was to check for errors related to non-volatile
variables.

> 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.

Sorry, I missed this scene. Maybe a comment is needed or we can split
the scene.

>
> >   #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.

IMO, efi_init_secure_state() is not just dealing with PK but also
includes Secure Boot mode transitions. So it may returns some
appropriate error when dealing with variable authentication.

>
> >
> >       /*
> >        * 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?

I think the ESP status (including fs corruption) is the most likely
situation to have an effect. But we should catch it earlier anyway.

BR,
Weizhao

>
> @Ilias: Please, chime in.
>
> Best regards
>
> Heinrich
>
> >
> > -     return EFI_SUCCESS;
> > +     return ret;
> >   }
> >
> >   efi_status_t efi_query_variable_info_int(u32 attributes,
>