Re: [PATCH v2 6/6] efi_loader: always initialize the secure boot state

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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