[PATCH] efi_loader: Fix UEFI variable error handling

[Ilias Apalodimas]

On Fri, 10 Nov 2023 at 15:12, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 10. November 2023 11:04:24 MEZ schrieb Ilias Apalodimas <ilias.apalodimas at linaro.org>:
> >Hi Heinrich, Weizhao
> >
> >On Thu, 9 Nov 2023 at 15:57, 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.
> >>
> >> 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.
> >
> >Yes, that's correct. The function description tries to explain that
> >but is a bit vague.
> >
> >>
> >> >   #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.
> >>
> >
> >Yea agree here
> >
> >> 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?
> >
> >What do you mean by old value?
>
> We are in SetVariable() and set, changed, or deleted PK in memory. But this has lead to some inconsistency. Should the prior state be restored?

Ah right.  As I said this code is basically there to allow us to run
self-tests. If setting the security state fails, those tests should
fail whether we revert or not. So I think we don't care. We could add
a comment explaining the situation with the secure boot state
transition when auth variables are not preseseded

Thanks
/Ilias


>
> Regards
>
> Heinrich
>
> >
> >> * Should we still persist PK to ubootefi.var?
> >
> >I would argue that we don't really care about what happens in this
> >case. Writing authenticated variables on a file is only supported if
> >preseeding is disabled and it *never* gets restored. We basically
> >allow that code to test EFI secure boot by writing PK, KEK, DB on the
> >fly, but once we reboot those variables are gone.
> >If preseeding is enabled we don't write that at all. We return
> >EFI_WRITE_PROTECTED.  We could do that regardless.  But since we have
> >those tests, the memory backend should still be allowed to write
> >those.
> >
> >>
> >> However we decide we should describe our decisions in a code comment.
> >
> >I think the logic here should be
> >1. If variables are preseeded and restoring any authenticated
> >variables fails, the EFI subsystem should refuse to start (which it
> >already does)
> >2. If preseeding is not configured we can continue as best effort and
> >try to recover the board and rewrite variables. We don't trust
> >variables stored in a file and we should keep it that way
> >
> >>
> >> >
> >> >       /*
> >> >        * 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.
> >
> >I think we should continue doing that *unless* preseeding is enabled.
> >If we preseed authenticated variables and the restoration of the
> >switching of the secure boot mode fails we must stop the EFI
> >subsystem.
> >The first function that gets called is efi_init_variables().
> >commit 77bb14758dcb1 explains some of the logic in that file but in short
> >- We load variables from a file first, excluding authenticated ones
> >- We load preseeded variables. That step allows you to load PK, KEK,
> >db etc that are stored as part of the u-boot binary.  We assume that
> >in a sane design a chain of trust *will* authenticate u-boot prior to
> >loading, so we 'trust' those variables.  If this fails which includes
> >setting the secure state we correctly stop the EFI subsystem
> >- efi_var_restore() is checking for duplicates. So a non-authenticated
> >variable set by the file load won't be overwritten by built-in ones.
> >This allows users to override built-in variables (apart from auth ones
> >obviously)
> >
> >Cheers
> >/Ilias
> >
> >
> >
> >
> >
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >> >
> >> > -     return EFI_SUCCESS;
> >> > +     return ret;
> >> >   }
> >> >
> >> >   efi_status_t efi_query_variable_info_int(u32 attributes,
> >>