Re: [PATCH v2 5/6] efi_loader: writing AuditMode, DeployedMode

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

On Fri, Aug 27, 2021 at 06:09:25AM +0200, Heinrich Schuchardt wrote:
> On 8/27/21 5:05 AM, AKASHI Takahiro wrote:
> > Heinrich,
> > 
> > On Thu, Aug 26, 2021 at 03:48:04PM +0200, Heinrich Schuchardt wrote:
> > > Writing variables AuditMode or Deployed Mode must update the secure boot
> > > state.
> > > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > > 	correct variable name in lib/efi_loader/efi_variable_tee.c
> > > ---
> > >   include/efi_variable.h            | 1 +
> > >   lib/efi_loader/efi_var_common.c   | 2 ++
> > >   lib/efi_loader/efi_variable.c     | 6 +++---
> > >   lib/efi_loader/efi_variable_tee.c | 4 +++-
> > >   4 files changed, 9 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > > index 2d97655e1f..0440d356bc 100644
> > > --- a/include/efi_variable.h
> > > +++ b/include/efi_variable.h
> > > @@ -12,6 +12,7 @@
> > > 
> > >   enum efi_auth_var_type {
> > >   	EFI_AUTH_VAR_NONE = 0,
> > > +	EFI_AUTH_MODE,
> > >   	EFI_AUTH_VAR_PK,
> > >   	EFI_AUTH_VAR_KEK,
> > >   	EFI_AUTH_VAR_DB,
> > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > > index 63ad6fea9e..6fabcfe72c 100644
> > > --- a/lib/efi_loader/efi_var_common.c
> > > +++ b/lib/efi_loader/efi_var_common.c
> > > @@ -34,6 +34,8 @@ static const struct efi_auth_var_name_type name_type[] = {
> > >   	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
> > >   	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
> > >   	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
> > > +	{u"AuditMode", &efi_global_variable_guid, EFI_AUTH_MODE},
> > > +	{u"DeployedMode", &efi_global_variable_guid, EFI_AUTH_MODE},
> > >   };
> > > 
> > >   static bool efi_secure_boot;
> > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > > index a7d305ffbc..80996d0f47 100644
> > > --- a/lib/efi_loader/efi_variable.c
> > > +++ b/lib/efi_loader/efi_variable.c
> > > @@ -247,7 +247,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> > >   			return EFI_WRITE_PROTECTED;
> > > 
> > >   		if (IS_ENABLED(CONFIG_EFI_VARIABLES_PRESEED)) {
> > > -			if (var_type != EFI_AUTH_VAR_NONE)
> > > +			if (var_type >= EFI_AUTH_VAR_PK)
> > 
> > This change is irrelevant to the purpose of this commit.
> 
> Thank you for reviewing the series.
> 
> EFI_AUTH_MODE is needed in the implementation of this patch and requires
> this change. But I can split the patch in two.
> 
> > 
> > >   				return EFI_WRITE_PROTECTED;
> > >   		}
> > > 
> > > @@ -268,7 +268,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> > >   			return EFI_NOT_FOUND;
> > >   	}
> > > 
> > > -	if (var_type != EFI_AUTH_VAR_NONE) {
> > > +	if (var_type >= EFI_AUTH_VAR_PK) {
> > >   		/* authentication is mandatory */
> > >   		if (!(attributes &
> > >   		      EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)) {
> > > @@ -328,7 +328,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> > >   	if (ret != EFI_SUCCESS)
> > >   		return ret;
> > > 
> > > -	if (var_type == EFI_AUTH_VAR_PK)
> > > +	if (var_type == EFI_AUTH_VAR_PK || var_type == EFI_AUTH_MODE)
> > >   		ret = efi_init_secure_state();
> > 
> > As I said, calling efi_init_secure_state() is not a good idea.
> > 
> > The scheme that I have in mind:
> > * if some event takes place, then trigger the transition.
> > * efi_transfer_secure_state() handles/take actions for the transition.
> > 
> > Looking at "Figure 32-4 Secure Boot Modes", there are a couple of events
> > defined:
> > 1) Enroll PKpub
> > 2) Platform Specific PKpub Clear/Delete PKpub
> > 3) Audit := 1
> > 4) DeployedMode := 1
> > 5) Platform Specific DeployedMode Clear
> > 
> > (Please note that "enroll/platform specific" operations should end up
> > modifying a relevant UEFI variable, any way.)
> > 
> > So, in the case above, we should do like this:
> >    if ("PK" is added/modified)
> >       if (SetupMode)
> >          efi_transfer_secure_state(UserMode)
> >       else (AuditMode)
> >          efi_transfer_secure_state(DeployedMode)
> >    else if ("AuditMode" is set)
> >       if (SetupMode || UserMode)
> >          efi_transfer_secure_state(AuditMode)
> >    else if
> >       and so on
> 
> Here we are in efi_set_variable_int(). efi_transfer_secure_state()
> itself calls efi_set_variable_int() repeatedly.
> 
> Hence we need a way for a user to call SetVariable() with the side
> effects you described above and a way to alter the state variables
> without side effects.
> 
> There are different ways to implement this:
> 
> 1) As we are on a single threaded system we can use a static
>    state variable. This is the approach in patch 1.
> 2) We can add a parameter to efi_set_variable_int() indicating that
>    the variable change shall not have side effects.
> 3) We can carve out a function for setting a variable without side
>    effects.
> 
> We have two implementations of efi_set_variable_int():
> 
> * One for file based variables in lib/efi_loader/efi_variable.c.
> * Another for StMM based variables in lib/efi_loader/efi_variable_tee.c.
> 
> Whatever we do must work for both implementations of variables.
> 
> lib/efi_loader/efi_variable_tee.c has two calls to
> efi_init_secure_state() currently matching the calls in
> lib/efi_loader/efi_variable.c.
> 
> @Ilias:
> Which part of the logic described in Figure 32-4 Secure Boot Modes of
> UEFI spec 2.9 exists inside StMM?

I found no support for the DeployedMode variable in the current upstream

> Where is it coded?
> 

It seems to be spread around EDK2 sources.

SecurityPkg/Library/AuthVariableLib/AuthVariableLib.c
SecurityPkg/Library/AuthVariableLib/AuthService.c
contain some info and are included in the source we use for the
StandaloneMM that we run from op-tee.

There's more references for SetupMode in EDK2 (in files that aren't included
in StMM) and commit 560ac77ea155 seems to have removed the support of what
you are looking for.

In any case I think the state machine and the logic should just be coded in
one place in U-Boot, regardless of the variable backend, and strictly use StMM for
storing/reading/authenticating variables.

Regards
/Ilias

> Best regards
> 
> Heinrich
> 
> > 
> > The logic is clear and the code directly renders what the figure 32-4 shows.
> > What's more, it will make it much easier for reviewers (and users)
> > to confirm the code is fully compliant with the specification
> > in terms of the "conditions" vs. resultant system status.
> > 
> > Then, each of the system's secure status can be always maintained
> > within efi_transfer_secure_state().
> > 
> > In addition, we will not have to have a hacky "lock" in
> > efi_init_secure_state().
> > 
> > Those are the reason why I want to stick to the scheme above.
> > 
> > -Takahiro Akashi
> > 
> > 
> > >   	else
> > >   		ret = EFI_SUCCESS;
> > > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > > index 51920bcb51..a6d5752045 100644
> > > --- a/lib/efi_loader/efi_variable_tee.c
> > > +++ b/lib/efi_loader/efi_variable_tee.c
> > > @@ -512,6 +512,7 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> > >   	efi_uintn_t payload_size;
> > >   	efi_uintn_t name_size;
> > >   	u8 *comm_buf = NULL;
> > > +	enum efi_auth_var_type var_type;
> > >   	bool ro;
> > > 
> > >   	if (!variable_name || variable_name[0] == 0 || !vendor) {
> > > @@ -590,7 +591,8 @@ efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> > >   	if (alt_ret != EFI_SUCCESS)
> > >   		goto out;
> > > 
> > > -	if (!u16_strcmp(variable_name, L"PK"))
> > > +	var_type = efi_auth_var_get_type(variable_name, vendor);
> > > +	if (var_type == EFI_AUTH_VAR_PK || var_type == EFI_AUTH_MODE)
> > >   		alt_ret = efi_init_secure_state();
> > >   out:
> > >   	free(comm_buf);
> > > --
> > > 2.30.2
> > > 
>