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

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

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