[PATCH v2 0/6] efi_loader: fix secure boot mode transitions

[PATCH v2 0/6] efi_loader: fix secure boot mode transitions

[Heinrich Schuchardt]

The UEFI specification 2.9 defines the different modes that secure boot may
be in. 

The patch series adds support for the "Deployed Mode" and the "Setup Mode".

Furthermore the secure boot signature database must only be loaded from
tamper-resistant storage. So we must not load it from ubootefi.var on the
EFI system partition but only from the preseed variables store or via the
OP-TEE driver for the eMMC replay protected memory partition.

v2:
	correct variable name in lib/efi_loader/efi_variable_tee.c

Heinrich Schuchardt (6):
  efi_loader: stop recursion in efi_init_secure_state
  efi_loader: correct determination of secure boot state
  efi_loader: don't load signature database from file
  efi_loader: correct secure boot state transition
  efi_loader: writing AuditMode, DeployedMode
  efi_loader: always initialize the secure boot state

 include/efi_variable.h            |  6 ++-
 lib/efi_loader/efi_var_common.c   | 66 +++++++++++++++++++++++--------
 lib/efi_loader/efi_var_file.c     | 41 +++++++++++--------
 lib/efi_loader/efi_variable.c     | 20 ++++++----
 lib/efi_loader/efi_variable_tee.c |  4 +-
 5 files changed, 95 insertions(+), 42 deletions(-)

-- 
2.30.2

[PATCH v2 1/6] efi_loader: stop recursion in efi_init_secure_state

[Heinrich Schuchardt]

efi_init_secure_state() calls efi_transfer_secure_state() which may delete
variable "PK" which will result in calling efi_init_secure_state() again.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 lib/efi_loader/efi_var_common.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index 3d92afe2eb..654ce81f9d 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -314,11 +314,15 @@ err:
 
 efi_status_t efi_init_secure_state(void)
 {
+	static bool lock;
 	enum efi_secure_mode mode = EFI_MODE_SETUP;
 	u8 efi_vendor_keys = 0;
 	efi_uintn_t size = 0;
 	efi_status_t ret;
 
+	if (lock)
+		return EFI_SUCCESS;
+
 	ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
 				   NULL, &size, NULL, NULL);
 	if (ret == EFI_BUFFER_TOO_SMALL) {
@@ -326,7 +330,9 @@ efi_status_t efi_init_secure_state(void)
 			mode = EFI_MODE_USER;
 	}
 
+	lock = true;
 	ret = efi_transfer_secure_state(mode);
+	lock = false;
 	if (ret != EFI_SUCCESS)
 		return ret;
 
-- 
2.30.2

[PATCH v2 2/6] efi_loader: correct determination of secure boot state

[Heinrich Schuchardt]

When U-Boot is started we have to use the existing variables to determine
in which secure boot state we are.

* If a platform key PK is present and DeployedMode=1, we are in deployed
  mode.
* If no platform key PK is present and AuditMode=1, we are in audit mode.
* Otherwise if a platform key is present, we are in user mode.
* Otherwise if no platform key is present, we are in setup mode.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 lib/efi_loader/efi_var_common.c | 39 ++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index 654ce81f9d..cf7afecd60 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -315,20 +315,43 @@ err:
 efi_status_t efi_init_secure_state(void)
 {
 	static bool lock;
-	enum efi_secure_mode mode = EFI_MODE_SETUP;
+	enum efi_secure_mode mode;
 	u8 efi_vendor_keys = 0;
-	efi_uintn_t size = 0;
+	efi_uintn_t size;
 	efi_status_t ret;
 
 	if (lock)
 		return EFI_SUCCESS;
-
-	ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
-				   NULL, &size, NULL, NULL);
-	if (ret == EFI_BUFFER_TOO_SMALL) {
-		if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT))
-			mode = EFI_MODE_USER;
+	u8 deployed_mode = 0;
+	u8 audit_mode = 0;
+	u8 setup_mode = 1;
+
+	if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT)) {
+		size = sizeof(deployed_mode);
+		ret = efi_get_variable_int(u"DeployedMode", &efi_global_variable_guid,
+					   NULL, &size, &deployed_mode, NULL);
+		size = sizeof(audit_mode);
+		ret = efi_get_variable_int(u"AuditMode", &efi_global_variable_guid,
+					   NULL, &size, &audit_mode, NULL);
+		size = 0;
+		ret = efi_get_variable_int(u"PK", &efi_global_variable_guid,
+					   NULL, &size, NULL, NULL);
+		if (ret == EFI_BUFFER_TOO_SMALL) {
+			setup_mode = 0;
+			audit_mode = 0;
+		} else {
+			setup_mode = 1;
+			deployed_mode = 0;
+		}
 	}
+	if (deployed_mode)
+		mode = EFI_MODE_DEPLOYED;
+	else if (audit_mode)
+		mode = EFI_MODE_AUDIT;
+	else if (setup_mode)
+		mode = EFI_MODE_SETUP;
+	else
+		mode = EFI_MODE_USER;
 
 	lock = true;
 	ret = efi_transfer_secure_state(mode);
-- 
2.30.2

[PATCH v2 3/6] efi_loader: don't load signature database from file

[Heinrich Schuchardt]

The UEFI specification requires that the signature database may only be
stored in tamper-resistant storage. So these variable may not be read
from an unsigned file.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 include/efi_variable.h          |  5 +++-
 lib/efi_loader/efi_var_common.c |  2 --
 lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
 lib/efi_loader/efi_variable.c   |  2 +-
 4 files changed, 30 insertions(+), 20 deletions(-)

diff --git a/include/efi_variable.h b/include/efi_variable.h
index 4623a64142..2d97655e1f 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
 /**
  * efi_var_restore() - restore EFI variables from buffer
  *
+ * Only if @safe is set secure boot related variables will be restored.
+ *
  * @buf:	buffer
+ * @safe:	restoring from tamper-resistant storage
  * Return:	status code
  */
-efi_status_t efi_var_restore(struct efi_var_file *buf);
+efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
 
 /**
  * efi_var_from_file() - read variables from file
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index cf7afecd60..b0c5b672c5 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
 	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
 	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
 	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
-	/* not used yet
 	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
 	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
-	*/
 };
 
 static bool efi_secure_boot;
diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
index de076b8cbc..c7c6805ed0 100644
--- a/lib/efi_loader/efi_var_file.c
+++ b/lib/efi_loader/efi_var_file.c
@@ -148,9 +148,10 @@ error:
 #endif
 }
 
-efi_status_t efi_var_restore(struct efi_var_file *buf)
+efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
 {
 	struct efi_var_entry *var, *last_var;
+	u16 *data;
 	efi_status_t ret;
 
 	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
@@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
 		return EFI_INVALID_PARAMETER;
 	}
 
-	var = buf->var;
 	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
-	while (var < last_var) {
-		u16 *data = var->name + u16_strlen(var->name) + 1;
-
-		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
-			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
-					      var->length, data, 0, NULL,
-					      var->time);
-			if (ret != EFI_SUCCESS)
-				log_err("Failed to set EFI variable %ls\n",
-					var->name);
-		}
-		var = (struct efi_var_entry *)
-		      ALIGN((uintptr_t)data + var->length, 8);
+	for (var = buf->var; var < last_var;
+	     var = (struct efi_var_entry *)
+		   ALIGN((uintptr_t)data + var->length, 8)) {
+
+		data = var->name + u16_strlen(var->name) + 1;
+
+		/*
+		 * Secure boot related and non-volatile variables shall only be
+		 * restored from U-Boot's preseed.
+		 */
+		if (!safe &&
+		    (efi_auth_var_get_type(var->name, &var->guid) !=
+		     EFI_AUTH_VAR_NONE ||
+		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
+			continue;
+		if (!var->length)
+			continue;
+		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
+				      var->length, data, 0, NULL,
+				      var->time);
+		if (ret != EFI_SUCCESS)
+			log_err("Failed to set EFI variable %ls\n", var->name);
 	}
 	return EFI_SUCCESS;
 }
@@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
 		log_err("Failed to load EFI variables\n");
 		goto error;
 	}
-	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
+	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
 		log_err("Invalid EFI variables file\n");
 error:
 	free(buf);
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index ba0874e9e7..a7d305ffbc 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -426,7 +426,7 @@ 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);
+				      __efi_var_file_begin, true);
 		if (ret != EFI_SUCCESS)
 			log_err("Invalid EFI variable seed\n");
 	}
-- 
2.30.2

[PATCH v2 4/6] efi_loader: correct secure boot state transition

[Heinrich Schuchardt]

Variable PK must be deleted when switching either to setup mode or to audit
mode.
Variable AuditMode must be writable in setup mode and user mode.
Variable DeployedMode must only be writable in user mode; simplify the
logic.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 lib/efi_loader/efi_var_common.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index b0c5b672c5..63ad6fea9e 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -240,7 +240,7 @@ static efi_status_t efi_set_secure_state(u8 secure_boot, u8 setup_mode,
 		goto err;
 
 	ret = efi_set_variable_int(L"AuditMode", &efi_global_variable_guid,
-				   audit_mode || setup_mode ?
+				   audit_mode || deployed_mode ?
 				   attributes_ro : attributes_rw,
 				   sizeof(audit_mode), &audit_mode, false);
 	if (ret != EFI_SUCCESS)
@@ -248,7 +248,7 @@ static efi_status_t efi_set_secure_state(u8 secure_boot, u8 setup_mode,
 
 	ret = efi_set_variable_int(L"DeployedMode",
 				   &efi_global_variable_guid,
-				   audit_mode || deployed_mode || setup_mode ?
+				   deployed_mode || setup_mode ?
 				   attributes_ro : attributes_rw,
 				   sizeof(deployed_mode), &deployed_mode,
 				   false);
@@ -273,17 +273,20 @@ static efi_status_t efi_transfer_secure_state(enum efi_secure_mode mode)
 	EFI_PRINT("Switching secure state from %d to %d\n", efi_secure_mode,
 		  mode);
 
-	if (mode == EFI_MODE_DEPLOYED) {
-		ret = efi_set_secure_state(1, 0, 0, 1);
-		if (ret != EFI_SUCCESS)
-			goto err;
-	} else if (mode == EFI_MODE_AUDIT) {
+	if (mode == EFI_MODE_SETUP || mode == EFI_MODE_AUDIT) {
 		ret = efi_set_variable_int(L"PK", &efi_global_variable_guid,
 					   EFI_VARIABLE_BOOTSERVICE_ACCESS |
 					   EFI_VARIABLE_RUNTIME_ACCESS,
 					   0, NULL, false);
+		if (ret != EFI_NOT_FOUND && ret != EFI_SUCCESS)
+			goto err;
+	}
+
+	if (mode == EFI_MODE_DEPLOYED) {
+		ret = efi_set_secure_state(1, 0, 0, 1);
 		if (ret != EFI_SUCCESS)
 			goto err;
+	} else if (mode == EFI_MODE_AUDIT) {
 
 		ret = efi_set_secure_state(0, 1, 1, 0);
 		if (ret != EFI_SUCCESS)
-- 
2.30.2

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

[Heinrich Schuchardt]

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)
 				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();
 	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

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

[Heinrich Schuchardt]

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();
+
+	return EFI_SUCCESS;
 }
-- 
2.30.2

Re: [PATCH v2 1/6] efi_loader: stop recursion in efi_init_secure_state

[AKASHI Takahiro]

Heinrich,

On Thu, Aug 26, 2021 at 03:48:00PM +0200, Heinrich Schuchardt wrote:
> efi_init_secure_state() calls efi_transfer_secure_state() which may delete
> variable "PK" which will result in calling efi_init_secure_state() again.

I don't think it is a right thing to do. So I would say nak to this version.
When I first implemented those functions, I intended to call
efi_init_secure_state() only at the system initialization.
Later on, all the transitions should be managed by efi_transfer_secure_state()
as well as its callers.

Calling efi_init_secure_state() in efi_set_variable_int() is a bad idea.
(then you see 'recursion'.)
I will explain more in your patch#5.

-Takahiro Akashi


> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	no change
> ---
>  lib/efi_loader/efi_var_common.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index 3d92afe2eb..654ce81f9d 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -314,11 +314,15 @@ err:
>  
>  efi_status_t efi_init_secure_state(void)
>  {
> +	static bool lock;
>  	enum efi_secure_mode mode = EFI_MODE_SETUP;
>  	u8 efi_vendor_keys = 0;
>  	efi_uintn_t size = 0;
>  	efi_status_t ret;
>  
> +	if (lock)
> +		return EFI_SUCCESS;
> +
>  	ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
>  				   NULL, &size, NULL, NULL);
>  	if (ret == EFI_BUFFER_TOO_SMALL) {
> @@ -326,7 +330,9 @@ efi_status_t efi_init_secure_state(void)
>  			mode = EFI_MODE_USER;
>  	}
>  
> +	lock = true;
>  	ret = efi_transfer_secure_state(mode);
> +	lock = false;
>  	if (ret != EFI_SUCCESS)
>  		return ret;
>  
> -- 
> 2.30.2
> 

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

[AKASHI Takahiro]

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
> 

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

[AKASHI Takahiro]

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.

- Takahiro Akashi

> +
> +	return EFI_SUCCESS;
>  }
> -- 
> 2.30.2
> 

Re: [PATCH v2 0/6] efi_loader: fix secure boot mode transitions

[AKASHI Takahiro]

On Thu, Aug 26, 2021 at 03:47:59PM +0200, Heinrich Schuchardt wrote:
> The UEFI specification 2.9 defines the different modes that secure boot may
> be in. 
> 
> The patch series adds support for the "Deployed Mode" and the "Setup Mode".

This sentence seems to be wrong, or at least inaccurate.
"Setup Mode" has been supported from the beginning when I implemented
secure boot. In other word, I implemented only the transition between
"Setup Mode" and "User Mode" only.

-Takahiro Akashi


> Furthermore the secure boot signature database must only be loaded from
> tamper-resistant storage. So we must not load it from ubootefi.var on the
> EFI system partition but only from the preseed variables store or via the
> OP-TEE driver for the eMMC replay protected memory partition.
> 
> v2:
> 	correct variable name in lib/efi_loader/efi_variable_tee.c
> 
> Heinrich Schuchardt (6):
>   efi_loader: stop recursion in efi_init_secure_state
>   efi_loader: correct determination of secure boot state
>   efi_loader: don't load signature database from file
>   efi_loader: correct secure boot state transition
>   efi_loader: writing AuditMode, DeployedMode
>   efi_loader: always initialize the secure boot state
> 
>  include/efi_variable.h            |  6 ++-
>  lib/efi_loader/efi_var_common.c   | 66 +++++++++++++++++++++++--------
>  lib/efi_loader/efi_var_file.c     | 41 +++++++++++--------
>  lib/efi_loader/efi_variable.c     | 20 ++++++----
>  lib/efi_loader/efi_variable_tee.c |  4 +-
>  5 files changed, 95 insertions(+), 42 deletions(-)
> 
> -- 
> 2.30.2
> 

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

[Heinrich Schuchardt]

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?
Where is it coded?

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
>>

Re: [PATCH v2 3/6] efi_loader: don't load signature database from file

[AKASHI Takahiro]

On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
> The UEFI specification requires that the signature database may only be
> stored in tamper-resistant storage. So these variable may not be read
> from an unsigned file.

I don't have a strong opinion here, but it seems to be too restrictive.
Nobody expects that file-based variable implementation is *safe*.
Leave it as it is so that people can easily experiment secure boot.

-Takahiro Akashi


> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	no change
> ---
>  include/efi_variable.h          |  5 +++-
>  lib/efi_loader/efi_var_common.c |  2 --
>  lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
>  lib/efi_loader/efi_variable.c   |  2 +-
>  4 files changed, 30 insertions(+), 20 deletions(-)
> 
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 4623a64142..2d97655e1f 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>  /**
>   * efi_var_restore() - restore EFI variables from buffer
>   *
> + * Only if @safe is set secure boot related variables will be restored.
> + *
>   * @buf:	buffer
> + * @safe:	restoring from tamper-resistant storage
>   * Return:	status code
>   */
> -efi_status_t efi_var_restore(struct efi_var_file *buf);
> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>  
>  /**
>   * efi_var_from_file() - read variables from file
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index cf7afecd60..b0c5b672c5 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
>  	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
>  	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
>  	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
> -	/* not used yet
>  	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
>  	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
> -	*/
>  };
>  
>  static bool efi_secure_boot;
> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> index de076b8cbc..c7c6805ed0 100644
> --- a/lib/efi_loader/efi_var_file.c
> +++ b/lib/efi_loader/efi_var_file.c
> @@ -148,9 +148,10 @@ error:
>  #endif
>  }
>  
> -efi_status_t efi_var_restore(struct efi_var_file *buf)
> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>  {
>  	struct efi_var_entry *var, *last_var;
> +	u16 *data;
>  	efi_status_t ret;
>  
>  	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
> @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
>  		return EFI_INVALID_PARAMETER;
>  	}
>  
> -	var = buf->var;
>  	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
> -	while (var < last_var) {
> -		u16 *data = var->name + u16_strlen(var->name) + 1;
> -
> -		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
> -			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> -					      var->length, data, 0, NULL,
> -					      var->time);
> -			if (ret != EFI_SUCCESS)
> -				log_err("Failed to set EFI variable %ls\n",
> -					var->name);
> -		}
> -		var = (struct efi_var_entry *)
> -		      ALIGN((uintptr_t)data + var->length, 8);
> +	for (var = buf->var; var < last_var;
> +	     var = (struct efi_var_entry *)
> +		   ALIGN((uintptr_t)data + var->length, 8)) {
> +
> +		data = var->name + u16_strlen(var->name) + 1;
> +
> +		/*
> +		 * Secure boot related and non-volatile variables shall only be
> +		 * restored from U-Boot's preseed.
> +		 */
> +		if (!safe &&
> +		    (efi_auth_var_get_type(var->name, &var->guid) !=
> +		     EFI_AUTH_VAR_NONE ||
> +		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
> +			continue;
> +		if (!var->length)
> +			continue;
> +		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> +				      var->length, data, 0, NULL,
> +				      var->time);
> +		if (ret != EFI_SUCCESS)
> +			log_err("Failed to set EFI variable %ls\n", var->name);
>  	}
>  	return EFI_SUCCESS;
>  }
> @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
>  		log_err("Failed to load EFI variables\n");
>  		goto error;
>  	}
> -	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
> +	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
>  		log_err("Invalid EFI variables file\n");
>  error:
>  	free(buf);
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index ba0874e9e7..a7d305ffbc 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -426,7 +426,7 @@ 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);
> +				      __efi_var_file_begin, true);
>  		if (ret != EFI_SUCCESS)
>  			log_err("Invalid EFI variable seed\n");
>  	}
> -- 
> 2.30.2
> 

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

[Heinrich Schuchardt]

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. 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 3/6] efi_loader: don't load signature database from file

[Heinrich Schuchardt]

On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
> On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
>> The UEFI specification requires that the signature database may only be
>> stored in tamper-resistant storage. So these variable may not be read
>> from an unsigned file.
>
> I don't have a strong opinion here, but it seems to be too restrictive.
> Nobody expects that file-based variable implementation is *safe*.
> Leave it as it is so that people can easily experiment secure boot.

If the prior boot stage checks the integrity of the U-Boot binary, the
file based implementation becomes 'safe' with this patch.

Users can still experiment with secure boot by setting the secure boot
variables via the efidebug command.

I cannot see a use case for having the secure boot data base on an
insecure medium.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>> 	no change
>> ---
>>   include/efi_variable.h          |  5 +++-
>>   lib/efi_loader/efi_var_common.c |  2 --
>>   lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
>>   lib/efi_loader/efi_variable.c   |  2 +-
>>   4 files changed, 30 insertions(+), 20 deletions(-)
>>
>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>> index 4623a64142..2d97655e1f 100644
>> --- a/include/efi_variable.h
>> +++ b/include/efi_variable.h
>> @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>>   /**
>>    * efi_var_restore() - restore EFI variables from buffer
>>    *
>> + * Only if @safe is set secure boot related variables will be restored.
>> + *
>>    * @buf:	buffer
>> + * @safe:	restoring from tamper-resistant storage
>>    * Return:	status code
>>    */
>> -efi_status_t efi_var_restore(struct efi_var_file *buf);
>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>>
>>   /**
>>    * efi_var_from_file() - read variables from file
>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>> index cf7afecd60..b0c5b672c5 100644
>> --- a/lib/efi_loader/efi_var_common.c
>> +++ b/lib/efi_loader/efi_var_common.c
>> @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
>>   	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
>>   	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
>>   	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
>> -	/* not used yet
>>   	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
>>   	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
>> -	*/
>>   };
>>
>>   static bool efi_secure_boot;
>> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
>> index de076b8cbc..c7c6805ed0 100644
>> --- a/lib/efi_loader/efi_var_file.c
>> +++ b/lib/efi_loader/efi_var_file.c
>> @@ -148,9 +148,10 @@ error:
>>   #endif
>>   }
>>
>> -efi_status_t efi_var_restore(struct efi_var_file *buf)
>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>>   {
>>   	struct efi_var_entry *var, *last_var;
>> +	u16 *data;
>>   	efi_status_t ret;
>>
>>   	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
>> @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
>>   		return EFI_INVALID_PARAMETER;
>>   	}
>>
>> -	var = buf->var;
>>   	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
>> -	while (var < last_var) {
>> -		u16 *data = var->name + u16_strlen(var->name) + 1;
>> -
>> -		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
>> -			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>> -					      var->length, data, 0, NULL,
>> -					      var->time);
>> -			if (ret != EFI_SUCCESS)
>> -				log_err("Failed to set EFI variable %ls\n",
>> -					var->name);
>> -		}
>> -		var = (struct efi_var_entry *)
>> -		      ALIGN((uintptr_t)data + var->length, 8);
>> +	for (var = buf->var; var < last_var;
>> +	     var = (struct efi_var_entry *)
>> +		   ALIGN((uintptr_t)data + var->length, 8)) {
>> +
>> +		data = var->name + u16_strlen(var->name) + 1;
>> +
>> +		/*
>> +		 * Secure boot related and non-volatile variables shall only be
>> +		 * restored from U-Boot's preseed.
>> +		 */
>> +		if (!safe &&
>> +		    (efi_auth_var_get_type(var->name, &var->guid) !=
>> +		     EFI_AUTH_VAR_NONE ||
>> +		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
>> +			continue;
>> +		if (!var->length)
>> +			continue;
>> +		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>> +				      var->length, data, 0, NULL,
>> +				      var->time);
>> +		if (ret != EFI_SUCCESS)
>> +			log_err("Failed to set EFI variable %ls\n", var->name);
>>   	}
>>   	return EFI_SUCCESS;
>>   }
>> @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
>>   		log_err("Failed to load EFI variables\n");
>>   		goto error;
>>   	}
>> -	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
>> +	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
>>   		log_err("Invalid EFI variables file\n");
>>   error:
>>   	free(buf);
>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>> index ba0874e9e7..a7d305ffbc 100644
>> --- a/lib/efi_loader/efi_variable.c
>> +++ b/lib/efi_loader/efi_variable.c
>> @@ -426,7 +426,7 @@ 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);
>> +				      __efi_var_file_begin, true);
>>   		if (ret != EFI_SUCCESS)
>>   			log_err("Invalid EFI variable seed\n");
>>   	}
>> --
>> 2.30.2
>>

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

[AKASHI Takahiro]

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

Re: [PATCH v2 3/6] efi_loader: don't load signature database from file

[AKASHI Takahiro]

On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
> On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
> > On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
> > > The UEFI specification requires that the signature database may only be
> > > stored in tamper-resistant storage. So these variable may not be read
> > > from an unsigned file.
> > 
> > I don't have a strong opinion here, but it seems to be too restrictive.
> > Nobody expects that file-based variable implementation is *safe*.
> > Leave it as it is so that people can easily experiment secure boot.
> 
> If the prior boot stage checks the integrity of the U-Boot binary, the
> file based implementation becomes 'safe' with this patch.

How safe (or secure) is it? That is a question.
What is your thread model?

-Takahiro Akashi


> Users can still experiment with secure boot by setting the secure boot
> variables via the efidebug command.
> 
> I cannot see a use case for having the secure boot data base on an
> insecure medium.
> 
> Best regards
> 
> Heinrich
> 
> > 
> > -Takahiro Akashi
> > 
> > 
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > ---
> > > v2:
> > > 	no change
> > > ---
> > >   include/efi_variable.h          |  5 +++-
> > >   lib/efi_loader/efi_var_common.c |  2 --
> > >   lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
> > >   lib/efi_loader/efi_variable.c   |  2 +-
> > >   4 files changed, 30 insertions(+), 20 deletions(-)
> > > 
> > > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > > index 4623a64142..2d97655e1f 100644
> > > --- a/include/efi_variable.h
> > > +++ b/include/efi_variable.h
> > > @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
> > >   /**
> > >    * efi_var_restore() - restore EFI variables from buffer
> > >    *
> > > + * Only if @safe is set secure boot related variables will be restored.
> > > + *
> > >    * @buf:	buffer
> > > + * @safe:	restoring from tamper-resistant storage
> > >    * Return:	status code
> > >    */
> > > -efi_status_t efi_var_restore(struct efi_var_file *buf);
> > > +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
> > > 
> > >   /**
> > >    * efi_var_from_file() - read variables from file
> > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > > index cf7afecd60..b0c5b672c5 100644
> > > --- a/lib/efi_loader/efi_var_common.c
> > > +++ b/lib/efi_loader/efi_var_common.c
> > > @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
> > >   	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
> > >   	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
> > >   	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
> > > -	/* not used yet
> > >   	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
> > >   	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
> > > -	*/
> > >   };
> > > 
> > >   static bool efi_secure_boot;
> > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> > > index de076b8cbc..c7c6805ed0 100644
> > > --- a/lib/efi_loader/efi_var_file.c
> > > +++ b/lib/efi_loader/efi_var_file.c
> > > @@ -148,9 +148,10 @@ error:
> > >   #endif
> > >   }
> > > 
> > > -efi_status_t efi_var_restore(struct efi_var_file *buf)
> > > +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
> > >   {
> > >   	struct efi_var_entry *var, *last_var;
> > > +	u16 *data;
> > >   	efi_status_t ret;
> > > 
> > >   	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
> > > @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
> > >   		return EFI_INVALID_PARAMETER;
> > >   	}
> > > 
> > > -	var = buf->var;
> > >   	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
> > > -	while (var < last_var) {
> > > -		u16 *data = var->name + u16_strlen(var->name) + 1;
> > > -
> > > -		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
> > > -			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> > > -					      var->length, data, 0, NULL,
> > > -					      var->time);
> > > -			if (ret != EFI_SUCCESS)
> > > -				log_err("Failed to set EFI variable %ls\n",
> > > -					var->name);
> > > -		}
> > > -		var = (struct efi_var_entry *)
> > > -		      ALIGN((uintptr_t)data + var->length, 8);
> > > +	for (var = buf->var; var < last_var;
> > > +	     var = (struct efi_var_entry *)
> > > +		   ALIGN((uintptr_t)data + var->length, 8)) {
> > > +
> > > +		data = var->name + u16_strlen(var->name) + 1;
> > > +
> > > +		/*
> > > +		 * Secure boot related and non-volatile variables shall only be
> > > +		 * restored from U-Boot's preseed.
> > > +		 */
> > > +		if (!safe &&
> > > +		    (efi_auth_var_get_type(var->name, &var->guid) !=
> > > +		     EFI_AUTH_VAR_NONE ||
> > > +		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
> > > +			continue;
> > > +		if (!var->length)
> > > +			continue;
> > > +		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> > > +				      var->length, data, 0, NULL,
> > > +				      var->time);
> > > +		if (ret != EFI_SUCCESS)
> > > +			log_err("Failed to set EFI variable %ls\n", var->name);
> > >   	}
> > >   	return EFI_SUCCESS;
> > >   }
> > > @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
> > >   		log_err("Failed to load EFI variables\n");
> > >   		goto error;
> > >   	}
> > > -	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
> > > +	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
> > >   		log_err("Invalid EFI variables file\n");
> > >   error:
> > >   	free(buf);
> > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > > index ba0874e9e7..a7d305ffbc 100644
> > > --- a/lib/efi_loader/efi_variable.c
> > > +++ b/lib/efi_loader/efi_variable.c
> > > @@ -426,7 +426,7 @@ 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);
> > > +				      __efi_var_file_begin, true);
> > >   		if (ret != EFI_SUCCESS)
> > >   			log_err("Invalid EFI variable seed\n");
> > >   	}
> > > --
> > > 2.30.2
> > > 
> 

Re: [PATCH v2 3/6] efi_loader: don't load signature database from file

[AKASHI Takahiro]

On Fri, Aug 27, 2021 at 01:49:41PM +0900, AKASHI Takahiro wrote:
> On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
> > On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
> > > On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
> > > > The UEFI specification requires that the signature database may only be
> > > > stored in tamper-resistant storage. So these variable may not be read
> > > > from an unsigned file.
> > > 
> > > I don't have a strong opinion here, but it seems to be too restrictive.
> > > Nobody expects that file-based variable implementation is *safe*.
> > > Leave it as it is so that people can easily experiment secure boot.
> > 
> > If the prior boot stage checks the integrity of the U-Boot binary, the
> > file based implementation becomes 'safe' with this patch.
> 
> How safe (or secure) is it? That is a question.
> What is your thread model?

Obviously, thread -> threat

> 
> -Takahiro Akashi
> 
> 
> > Users can still experiment with secure boot by setting the secure boot
> > variables via the efidebug command.
> > 
> > I cannot see a use case for having the secure boot data base on an
> > insecure medium.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > 
> > > -Takahiro Akashi
> > > 
> > > 
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > ---
> > > > v2:
> > > > 	no change
> > > > ---
> > > >   include/efi_variable.h          |  5 +++-
> > > >   lib/efi_loader/efi_var_common.c |  2 --
> > > >   lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
> > > >   lib/efi_loader/efi_variable.c   |  2 +-
> > > >   4 files changed, 30 insertions(+), 20 deletions(-)
> > > > 
> > > > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > > > index 4623a64142..2d97655e1f 100644
> > > > --- a/include/efi_variable.h
> > > > +++ b/include/efi_variable.h
> > > > @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
> > > >   /**
> > > >    * efi_var_restore() - restore EFI variables from buffer
> > > >    *
> > > > + * Only if @safe is set secure boot related variables will be restored.
> > > > + *
> > > >    * @buf:	buffer
> > > > + * @safe:	restoring from tamper-resistant storage
> > > >    * Return:	status code
> > > >    */
> > > > -efi_status_t efi_var_restore(struct efi_var_file *buf);
> > > > +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
> > > > 
> > > >   /**
> > > >    * efi_var_from_file() - read variables from file
> > > > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > > > index cf7afecd60..b0c5b672c5 100644
> > > > --- a/lib/efi_loader/efi_var_common.c
> > > > +++ b/lib/efi_loader/efi_var_common.c
> > > > @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
> > > >   	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
> > > >   	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
> > > >   	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
> > > > -	/* not used yet
> > > >   	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
> > > >   	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
> > > > -	*/
> > > >   };
> > > > 
> > > >   static bool efi_secure_boot;
> > > > diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
> > > > index de076b8cbc..c7c6805ed0 100644
> > > > --- a/lib/efi_loader/efi_var_file.c
> > > > +++ b/lib/efi_loader/efi_var_file.c
> > > > @@ -148,9 +148,10 @@ error:
> > > >   #endif
> > > >   }
> > > > 
> > > > -efi_status_t efi_var_restore(struct efi_var_file *buf)
> > > > +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
> > > >   {
> > > >   	struct efi_var_entry *var, *last_var;
> > > > +	u16 *data;
> > > >   	efi_status_t ret;
> > > > 
> > > >   	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
> > > > @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
> > > >   		return EFI_INVALID_PARAMETER;
> > > >   	}
> > > > 
> > > > -	var = buf->var;
> > > >   	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
> > > > -	while (var < last_var) {
> > > > -		u16 *data = var->name + u16_strlen(var->name) + 1;
> > > > -
> > > > -		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
> > > > -			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> > > > -					      var->length, data, 0, NULL,
> > > > -					      var->time);
> > > > -			if (ret != EFI_SUCCESS)
> > > > -				log_err("Failed to set EFI variable %ls\n",
> > > > -					var->name);
> > > > -		}
> > > > -		var = (struct efi_var_entry *)
> > > > -		      ALIGN((uintptr_t)data + var->length, 8);
> > > > +	for (var = buf->var; var < last_var;
> > > > +	     var = (struct efi_var_entry *)
> > > > +		   ALIGN((uintptr_t)data + var->length, 8)) {
> > > > +
> > > > +		data = var->name + u16_strlen(var->name) + 1;
> > > > +
> > > > +		/*
> > > > +		 * Secure boot related and non-volatile variables shall only be
> > > > +		 * restored from U-Boot's preseed.
> > > > +		 */
> > > > +		if (!safe &&
> > > > +		    (efi_auth_var_get_type(var->name, &var->guid) !=
> > > > +		     EFI_AUTH_VAR_NONE ||
> > > > +		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
> > > > +			continue;
> > > > +		if (!var->length)
> > > > +			continue;
> > > > +		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
> > > > +				      var->length, data, 0, NULL,
> > > > +				      var->time);
> > > > +		if (ret != EFI_SUCCESS)
> > > > +			log_err("Failed to set EFI variable %ls\n", var->name);
> > > >   	}
> > > >   	return EFI_SUCCESS;
> > > >   }
> > > > @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
> > > >   		log_err("Failed to load EFI variables\n");
> > > >   		goto error;
> > > >   	}
> > > > -	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
> > > > +	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
> > > >   		log_err("Invalid EFI variables file\n");
> > > >   error:
> > > >   	free(buf);
> > > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > > > index ba0874e9e7..a7d305ffbc 100644
> > > > --- a/lib/efi_loader/efi_variable.c
> > > > +++ b/lib/efi_loader/efi_variable.c
> > > > @@ -426,7 +426,7 @@ 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);
> > > > +				      __efi_var_file_begin, true);
> > > >   		if (ret != EFI_SUCCESS)
> > > >   			log_err("Invalid EFI variable seed\n");
> > > >   	}
> > > > --
> > > > 2.30.2
> > > > 
> > 

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

[Heinrich Schuchardt]

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

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 3/6] efi_loader: don't load signature database from file

[Heinrich Schuchardt]

On 8/27/21 6:49 AM, AKASHI Takahiro wrote:
> On Fri, Aug 27, 2021 at 06:42:39AM +0200, Heinrich Schuchardt wrote:
>> On 8/27/21 6:12 AM, AKASHI Takahiro wrote:
>>> On Thu, Aug 26, 2021 at 03:48:02PM +0200, Heinrich Schuchardt wrote:
>>>> The UEFI specification requires that the signature database may only be
>>>> stored in tamper-resistant storage. So these variable may not be read
>>>> from an unsigned file.
>>>
>>> I don't have a strong opinion here, but it seems to be too restrictive.
>>> Nobody expects that file-based variable implementation is *safe*.
>>> Leave it as it is so that people can easily experiment secure boot.
>>
>> If the prior boot stage checks the integrity of the U-Boot binary, the
>> file based implementation becomes 'safe' with this patch.
>
> How safe (or secure) is it? That is a question.
> What is your thread model?

The preseed store is as safe as the capsule updates that Linaro is
working on where the certificate for verifying the capsule is baked into
U-Boot or the StMM based variables.

They all require that an attacker can neither load a manipulated U-Boot
nor that he can alter the memory containing U-Boot at runtime.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> Users can still experiment with secure boot by setting the secure boot
>> variables via the efidebug command.
>>
>> I cannot see a use case for having the secure boot data base on an
>> insecure medium.
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> -Takahiro Akashi
>>>
>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v2:
>>>> 	no change
>>>> ---
>>>>    include/efi_variable.h          |  5 +++-
>>>>    lib/efi_loader/efi_var_common.c |  2 --
>>>>    lib/efi_loader/efi_var_file.c   | 41 ++++++++++++++++++++-------------
>>>>    lib/efi_loader/efi_variable.c   |  2 +-
>>>>    4 files changed, 30 insertions(+), 20 deletions(-)
>>>>
>>>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>>>> index 4623a64142..2d97655e1f 100644
>>>> --- a/include/efi_variable.h
>>>> +++ b/include/efi_variable.h
>>>> @@ -161,10 +161,13 @@ efi_status_t __maybe_unused efi_var_collect(struct efi_var_file **bufp, loff_t *
>>>>    /**
>>>>     * efi_var_restore() - restore EFI variables from buffer
>>>>     *
>>>> + * Only if @safe is set secure boot related variables will be restored.
>>>> + *
>>>>     * @buf:	buffer
>>>> + * @safe:	restoring from tamper-resistant storage
>>>>     * Return:	status code
>>>>     */
>>>> -efi_status_t efi_var_restore(struct efi_var_file *buf);
>>>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe);
>>>>
>>>>    /**
>>>>     * efi_var_from_file() - read variables from file
>>>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>>>> index cf7afecd60..b0c5b672c5 100644
>>>> --- a/lib/efi_loader/efi_var_common.c
>>>> +++ b/lib/efi_loader/efi_var_common.c
>>>> @@ -32,10 +32,8 @@ static const struct efi_auth_var_name_type name_type[] = {
>>>>    	{u"KEK", &efi_global_variable_guid, EFI_AUTH_VAR_KEK},
>>>>    	{u"db",  &efi_guid_image_security_database, EFI_AUTH_VAR_DB},
>>>>    	{u"dbx",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBX},
>>>> -	/* not used yet
>>>>    	{u"dbt",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBT},
>>>>    	{u"dbr",  &efi_guid_image_security_database, EFI_AUTH_VAR_DBR},
>>>> -	*/
>>>>    };
>>>>
>>>>    static bool efi_secure_boot;
>>>> diff --git a/lib/efi_loader/efi_var_file.c b/lib/efi_loader/efi_var_file.c
>>>> index de076b8cbc..c7c6805ed0 100644
>>>> --- a/lib/efi_loader/efi_var_file.c
>>>> +++ b/lib/efi_loader/efi_var_file.c
>>>> @@ -148,9 +148,10 @@ error:
>>>>    #endif
>>>>    }
>>>>
>>>> -efi_status_t efi_var_restore(struct efi_var_file *buf)
>>>> +efi_status_t efi_var_restore(struct efi_var_file *buf, bool safe)
>>>>    {
>>>>    	struct efi_var_entry *var, *last_var;
>>>> +	u16 *data;
>>>>    	efi_status_t ret;
>>>>
>>>>    	if (buf->reserved || buf->magic != EFI_VAR_FILE_MAGIC ||
>>>> @@ -160,21 +161,29 @@ efi_status_t efi_var_restore(struct efi_var_file *buf)
>>>>    		return EFI_INVALID_PARAMETER;
>>>>    	}
>>>>
>>>> -	var = buf->var;
>>>>    	last_var = (struct efi_var_entry *)((u8 *)buf + buf->length);
>>>> -	while (var < last_var) {
>>>> -		u16 *data = var->name + u16_strlen(var->name) + 1;
>>>> -
>>>> -		if (var->attr & EFI_VARIABLE_NON_VOLATILE && var->length) {
>>>> -			ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>>>> -					      var->length, data, 0, NULL,
>>>> -					      var->time);
>>>> -			if (ret != EFI_SUCCESS)
>>>> -				log_err("Failed to set EFI variable %ls\n",
>>>> -					var->name);
>>>> -		}
>>>> -		var = (struct efi_var_entry *)
>>>> -		      ALIGN((uintptr_t)data + var->length, 8);
>>>> +	for (var = buf->var; var < last_var;
>>>> +	     var = (struct efi_var_entry *)
>>>> +		   ALIGN((uintptr_t)data + var->length, 8)) {
>>>> +
>>>> +		data = var->name + u16_strlen(var->name) + 1;
>>>> +
>>>> +		/*
>>>> +		 * Secure boot related and non-volatile variables shall only be
>>>> +		 * restored from U-Boot's preseed.
>>>> +		 */
>>>> +		if (!safe &&
>>>> +		    (efi_auth_var_get_type(var->name, &var->guid) !=
>>>> +		     EFI_AUTH_VAR_NONE ||
>>>> +		     !(var->attr & EFI_VARIABLE_NON_VOLATILE)))
>>>> +			continue;
>>>> +		if (!var->length)
>>>> +			continue;
>>>> +		ret = efi_var_mem_ins(var->name, &var->guid, var->attr,
>>>> +				      var->length, data, 0, NULL,
>>>> +				      var->time);
>>>> +		if (ret != EFI_SUCCESS)
>>>> +			log_err("Failed to set EFI variable %ls\n", var->name);
>>>>    	}
>>>>    	return EFI_SUCCESS;
>>>>    }
>>>> @@ -213,7 +222,7 @@ efi_status_t efi_var_from_file(void)
>>>>    		log_err("Failed to load EFI variables\n");
>>>>    		goto error;
>>>>    	}
>>>> -	if (buf->length != len || efi_var_restore(buf) != EFI_SUCCESS)
>>>> +	if (buf->length != len || efi_var_restore(buf, false) != EFI_SUCCESS)
>>>>    		log_err("Invalid EFI variables file\n");
>>>>    error:
>>>>    	free(buf);
>>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>>>> index ba0874e9e7..a7d305ffbc 100644
>>>> --- a/lib/efi_loader/efi_variable.c
>>>> +++ b/lib/efi_loader/efi_variable.c
>>>> @@ -426,7 +426,7 @@ 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);
>>>> +				      __efi_var_file_begin, true);
>>>>    		if (ret != EFI_SUCCESS)
>>>>    			log_err("Invalid EFI variable seed\n");
>>>>    	}
>>>> --
>>>> 2.30.2
>>>>
>>

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

[Ilias Apalodimas]

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
> > > 
>