All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 0/3] efi_loader: secure boot using preseed cert db
@ 2021-09-02  9:35 Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 1/3] efi_loader: don't load signature database from file Heinrich Schuchardt
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-09-02  9:35 UTC (permalink / raw
  To: u-boot
  Cc: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
	AKASHI Takahiro, Heinrich Schuchardt

When implementing secure boot the database with the certificates must be
stored in tamper-resistant storage. This implies it cannot be read from
a file on the EFI system partition.

We already have the possibility to provide UEFI variables built into
U-Boot via CONFIG_EFI_VAR_SEED_FILE. If TF-A validates BL33 alias U-Boot,
this seems adequate for secure boot.

With the patch series reading or changing the certificate database is
disabled. Furthermore the variable AuditMode and DeployedMode cannot be
read from file.

The series has been split of
[PATCH v2 0/6] efi_loader: fix secure boot mode transitions
https://lists.denx.de/pipermail/u-boot/2021-August/459054.html
because the implementation of Secure Boot mode transitions need more
thought.

Heinrich Schuchardt (3):
  efi_loader: don't load signature database from file
  efi_loader: efi_auth_var_type for AuditMode, DeployedMode
  efi_loader: correct determination of secure boot state

 include/efi_variable.h          |  6 ++++-
 lib/efi_loader/efi_var_common.c | 43 +++++++++++++++++++++++++--------
 lib/efi_loader/efi_var_file.c   | 41 +++++++++++++++++++------------
 lib/efi_loader/efi_variable.c   |  6 ++---
 4 files changed, 66 insertions(+), 30 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/3] efi_loader: don't load signature database from file
  2021-09-02  9:35 [PATCH v3 0/3] efi_loader: secure boot using preseed cert db Heinrich Schuchardt
@ 2021-09-02  9:35 ` Heinrich Schuchardt
  2021-09-06  0:12   ` AKASHI Takahiro
  2021-09-02  9:35 ` [PATCH v3 2/3] efi_loader: efi_auth_var_type for AuditMode, DeployedMode Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 3/3] efi_loader: correct determination of secure boot state Heinrich Schuchardt
  2 siblings, 1 reply; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-09-02  9:35 UTC (permalink / raw
  To: u-boot
  Cc: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
	AKASHI Takahiro, 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 3d92afe2eb..005c03ea5f 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.32.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] efi_loader: efi_auth_var_type for AuditMode, DeployedMode
  2021-09-02  9:35 [PATCH v3 0/3] efi_loader: secure boot using preseed cert db Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 1/3] efi_loader: don't load signature database from file Heinrich Schuchardt
@ 2021-09-02  9:35 ` Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 3/3] efi_loader: correct determination of secure boot state Heinrich Schuchardt
  2 siblings, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-09-02  9:35 UTC (permalink / raw
  To: u-boot
  Cc: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
	AKASHI Takahiro, Heinrich Schuchardt

Writing variables AuditMode and DeployedMode serves to switch between
Secure Boot modes. Provide a separate value for these in efi_auth_var_type.

With this patch the variables will not be read from from file even if they
are marked as non-volatile by mistake.

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   | 4 ++--
 3 files changed, 5 insertions(+), 2 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 005c03ea5f..c744e2fd91 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..fa2b6bc7a8 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)) {
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] efi_loader: correct determination of secure boot state
  2021-09-02  9:35 [PATCH v3 0/3] efi_loader: secure boot using preseed cert db Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 1/3] efi_loader: don't load signature database from file Heinrich Schuchardt
  2021-09-02  9:35 ` [PATCH v3 2/3] efi_loader: efi_auth_var_type for AuditMode, DeployedMode Heinrich Schuchardt
@ 2021-09-02  9:35 ` Heinrich Schuchardt
  2 siblings, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-09-02  9:35 UTC (permalink / raw
  To: u-boot
  Cc: Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas,
	AKASHI Takahiro

From: Heinrich Schuchardt <xypron.glpk@gmx.de>

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 <xypron.glpk@gmx.de>
---
 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 c744e2fd91..a00bbf1620 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -314,17 +314,40 @@ err:
 
 efi_status_t efi_init_secure_state(void)
 {
-	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;
-
-	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;
 
 	ret = efi_transfer_secure_state(mode);
 	if (ret != EFI_SUCCESS)
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/3] efi_loader: don't load signature database from file
  2021-09-02  9:35 ` [PATCH v3 1/3] efi_loader: don't load signature database from file Heinrich Schuchardt
@ 2021-09-06  0:12   ` AKASHI Takahiro
  2021-09-06  6:59     ` Heinrich Schuchardt
  0 siblings, 1 reply; 6+ messages in thread
From: AKASHI Takahiro @ 2021-09-06  0:12 UTC (permalink / raw
  To: Heinrich Schuchardt
  Cc: u-boot, Heinrich Schuchardt, Alexander Graf, Ilias Apalodimas

Heinrich,

On Thu, Sep 02, 2021 at 11:35:29AM +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.

Even with TF-A (or other methods) assumed, I think the behavior here is
too restrictive.

If you think TF-A provides the base for "chain of trust", all what you
need to do is to protect PK and all the other auth variables should be
allowed to be restored even from an unsafe file because the changes of
values will be verified anyway by UEFI system as long as SetVariable()
is used.

Please think about why UEFI specification defines both PK and KEK.
The ability of setting/modifying KEK will add more flexibility of system
configuration.

-Takahiro Akashi


> 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 3d92afe2eb..005c03ea5f 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.32.0
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/3] efi_loader: don't load signature database from file
  2021-09-06  0:12   ` AKASHI Takahiro
@ 2021-09-06  6:59     ` Heinrich Schuchardt
  0 siblings, 0 replies; 6+ messages in thread
From: Heinrich Schuchardt @ 2021-09-06  6:59 UTC (permalink / raw
  To: AKASHI Takahiro; +Cc: Alexander Graf, u-boot, Ilias Apalodimas



On 9/6/21 2:12 AM, AKASHI Takahiro wrote:
> Heinrich,
>
> On Thu, Sep 02, 2021 at 11:35:29AM +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.
>
> Even with TF-A (or other methods) assumed, I think the behavior here is
> too restrictive.
>
> If you think TF-A provides the base for "chain of trust", all what you
> need to do is to protect PK and all the other auth variables should be
> allowed to be restored even from an unsafe file because the changes of
> values will be verified anyway by UEFI system as long as SetVariable()
> is used.
>
> Please think about why UEFI specification defines both PK and KEK.
> The ability of setting/modifying KEK will add more flexibility of system
> configuration.

If relying on only PK being fixed, one would still have to get the
sequence of import from file right: PK -> KEK -> db,dbx,dbt,dbr.

This would require a change in the file loader (efi_var_restore()).
Could you contribute such a patch?

Best regards

Heinrich

>
> -Takahiro Akashi
>
>
>> 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 3d92afe2eb..005c03ea5f 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.32.0
>>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-09-06  6:59 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-09-02  9:35 [PATCH v3 0/3] efi_loader: secure boot using preseed cert db Heinrich Schuchardt
2021-09-02  9:35 ` [PATCH v3 1/3] efi_loader: don't load signature database from file Heinrich Schuchardt
2021-09-06  0:12   ` AKASHI Takahiro
2021-09-06  6:59     ` Heinrich Schuchardt
2021-09-02  9:35 ` [PATCH v3 2/3] efi_loader: efi_auth_var_type for AuditMode, DeployedMode Heinrich Schuchardt
2021-09-02  9:35 ` [PATCH v3 3/3] efi_loader: correct determination of secure boot state Heinrich Schuchardt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.