On Thu, Nov 9, 2023 at 9:57 PM Heinrich Schuchardt <xypron.g...@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 <o451686...@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,
>
