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

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

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
>