Re: [PATCH v2 6/6] efi_loader: always initialize the secure boot state
On 8/27/21 6:47 AM, AKASHI Takahiro wrote:
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
---
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.
The UEFI spec requires that the secure boot database is stored in
tamper-resistant storage. So it cannot be on the ESP.
Best regards
Heinrich
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
Re: [PATCH v2 6/6] efi_loader: always initialize the secure boot state
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
> > > ---
> > > 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
Re: [PATCH v2 6/6] efi_loader: always initialize the secure boot state
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
---
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. If this
store is inconsistent, failing is required to ensure secure boot.
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
Re: [PATCH v2 6/6] efi_loader: always initialize the secure boot state
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
> ---
> 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.
- Takahiro Akashi
> +
> + return EFI_SUCCESS;
> }
> --
> 2.30.2
>
