On Fri, Aug 27, 2021 at 06:34:30AM +0200, Heinrich Schuchardt wrote:
> On 8/27/21 5:53 AM, AKASHI Takahiro wrote:
> > On Thu, Aug 26, 2021 at 03:48:05PM +0200, Heinrich Schuchardt wrote:
> > > Even if we cannot read the variable store from disk we still need to
> > > initialize the secure boot state.
> > >
> > > Don't continue to boot if the variable preseed is invalid as this
> > > indicates
> > > that the variable store has been tampered.
> > >
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
> > > ---
> > > v2:
> > > no change
> > > ---
> > > lib/efi_loader/efi_variable.c | 12 ++++++++----
> > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > > index 80996d0f47..6d92229e2a 100644
> > > --- a/lib/efi_loader/efi_variable.c
> > > +++ b/lib/efi_loader/efi_variable.c
> > > @@ -427,13 +427,17 @@ efi_status_t efi_init_variables(void)
> > > if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
> > > ret = efi_var_restore((struct efi_var_file *)
> > > __efi_var_file_begin, true);
> > > - if (ret != EFI_SUCCESS)
> > > + if (ret != EFI_SUCCESS) {
> > > log_err("Invalid EFI variable seed\n");
> > > + return ret;
> > > + }
> > > }
> > > -
> > > - ret = efi_var_from_file();
> > > + ret = efi_init_secure_state();
> > > if (ret != EFI_SUCCESS)
> > > return ret;
> > >
> > > - return efi_init_secure_state();
> > > + /* Don't stop booting if variable store is not available */
> > > + efi_var_from_file();
> >
> > I think we have to think about two different cases:
> > 1) there is no "variable store" file available.
> > 2) it does exists, but reading from it (efi_var_restore()) failed
> >
> > For (2), we should return with an error as in the case of
> > CONFIG_EFI_VARIABLES_PRESEED.
> > Otherwise, the behavior is inconsistent.
>
> The preseed store is used for secure boot related variables.
Where does this restriction come from?
Kconfig says:
Include a file with the initial values for non-volatile UEFI variables
into the U-Boot binary. If this configuration option is set, changes
to authentication related variables (PK, KEK, db, dbx) are not
allowed.
# oops: I didn't notice the last sentence.
# but anyhow, it seems too restrictive.
> If this
> store is inconsistent, failing is required to ensure secure boot.
As I said, a file-based variable configuration cannot work
as a secure platform any way.
Why do we need this kind of restriction?
-Takahiro Akashi
> With my patches applied the file store can not contain any secure boot
> related variables but it may contain boot options.
>
> Your suggestion could mean that if the file store is corrupted the board
> is bricked.
>
> If the boot options cannot be read either because the file does not
> exist or because the file is corrupt, I still want the user to have a
> chance to load shim, GRUB, or the kernel and boot via the bootefi command.
>
> I cannot see any inconsistency here.
>
> Best regards
>
> Heinrich
