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

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

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.

>  				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

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
>