[PATCH v3 0/4] efi_loader: centralize known vendor GUIDs

[PATCH v3 0/4] efi_loader: centralize known vendor GUIDs

[Heinrich Schuchardt]

The UEFI specification defines which vendor GUIDs should be used for
predefined variables like 'PK'. Currently we have multiple places
where this relationship is stored.

With this patch series a function for retrieving the GUID is provided
and existing code is adjusted to used it.

v3:
	Keep error handling in efi_sigstore_parse_sigdb()
v2:
	Remove a superfluous value check.
	Adjust commit messages and comments in the code.

Heinrich Schuchardt (4):
  efi_loader: treat UEFI variable name as const
  efi_loader: function to get GUID for variable name
  efi_loader: simplify efi_sigstore_parse_sigdb()
  efi_loader: simplify tcg2_measure_secure_boot_variable()

 include/efi_loader.h              |  2 +-
 include/efi_variable.h            | 27 +++++++++++++++++++++------
 lib/efi_loader/efi_signature.c    | 11 ++---------
 lib/efi_loader/efi_tcg2.c         | 31 ++++++++++++++-----------------
 lib/efi_loader/efi_var_common.c   | 14 ++++++++++++--
 lib/efi_loader/efi_var_mem.c      |  7 ++++---
 lib/efi_loader/efi_variable.c     |  9 +++++----
 lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------
 8 files changed, 69 insertions(+), 48 deletions(-)

-- 
2.32.0

[PATCH v3 1/4] efi_loader: treat UEFI variable name as const

[Heinrich Schuchardt]

UEFI variable names are typically constants and hence should be defined as
const. Unfortunately some of our API functions do not define the parameters
for UEFI variable names as const. This requires unnecessary conversions.

Adjust parameters of several internal functions to tre UEFI variable names
as const.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	no change
v2:
	adjust commit message
---
 include/efi_loader.h              |  2 +-
 include/efi_variable.h            | 16 ++++++++++------
 lib/efi_loader/efi_tcg2.c         |  2 +-
 lib/efi_loader/efi_var_common.c   |  5 +++--
 lib/efi_loader/efi_var_mem.c      |  7 ++++---
 lib/efi_loader/efi_variable.c     |  9 +++++----
 lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------
 7 files changed, 34 insertions(+), 23 deletions(-)

diff --git a/include/efi_loader.h b/include/efi_loader.h
index c440962fe5..125052d002 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -816,7 +816,7 @@ efi_status_t EFIAPI efi_query_variable_info(
 			u64 *remaining_variable_storage_size,
 			u64 *maximum_variable_size);
 
-void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
+void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
 
 /*
  * See section 3.1.3 in the v2.7 UEFI spec for more details on
diff --git a/include/efi_variable.h b/include/efi_variable.h
index 0440d356bc..8f666b2309 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -32,7 +32,8 @@ enum efi_auth_var_type {
  * @timep:		authentication time (seconds since start of epoch)
  * Return:		status code
  */
-efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+efi_status_t efi_get_variable_int(const u16 *variable_name,
+				  const efi_guid_t *vendor,
 				  u32 *attributes, efi_uintn_t *data_size,
 				  void *data, u64 *timep);
 
@@ -47,7 +48,8 @@ efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
  * @ro_check:		check the read only read only bit in attributes
  * Return:		status code
  */
-efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+efi_status_t efi_set_variable_int(const u16 *variable_name,
+				  const efi_guid_t *vendor,
 				  u32 attributes, efi_uintn_t data_size,
 				  const void *data, bool ro_check);
 
@@ -224,7 +226,7 @@ void efi_var_mem_del(struct efi_var_entry *var);
  * @time:		time of authentication (as seconds since start of epoch)
  * Result:		status code
  */
-efi_status_t efi_var_mem_ins(u16 *variable_name,
+efi_status_t efi_var_mem_ins(const u16 *variable_name,
 			     const efi_guid_t *vendor, u32 attributes,
 			     const efi_uintn_t size1, const void *data1,
 			     const efi_uintn_t size2, const void *data2,
@@ -251,7 +253,8 @@ efi_status_t efi_init_secure_state(void);
  * @guid:	guid of UEFI variable
  * Return:	identifier for authentication related variables
  */
-enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid);
+enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
+					     const efi_guid_t *guid);
 
 /**
  * efi_get_next_variable_name_mem() - Runtime common code across efi variable
@@ -280,8 +283,9 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
  * Return:		status code
  */
 efi_status_t __efi_runtime
-efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
-		     efi_uintn_t *data_size, void *data, u64 *timep);
+efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
+		     u32 *attributes, efi_uintn_t *data_size, void *data,
+		     u64 *timep);
 
 /**
  * efi_get_variable_runtime() - runtime implementation of GetVariable()
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index d3b8f93f14..ed1506012b 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -1365,7 +1365,7 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev)
  * Return:	status code
  */
 static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
-					  u32 event_type, u16 *var_name,
+					  u32 event_type, const u16 *var_name,
 					  const efi_guid_t *guid,
 					  efi_uintn_t data_size, u8 *data)
 {
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index a00bbf1620..e179932124 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -374,7 +374,8 @@ bool efi_secure_boot_enabled(void)
 	return efi_secure_boot;
 }
 
-enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid)
+enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
+					     const efi_guid_t *guid)
 {
 	for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) {
 		if (!u16_strcmp(name, name_type[i].name) &&
@@ -393,7 +394,7 @@ enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid)
  *
  * Return:	buffer with variable data or NULL
  */
-void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
+void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
 {
 	efi_status_t ret;
 	void *buf = NULL;
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
index 3d335a8274..13909b1d26 100644
--- a/lib/efi_loader/efi_var_mem.c
+++ b/lib/efi_loader/efi_var_mem.c
@@ -134,7 +134,7 @@ void __efi_runtime efi_var_mem_del(struct efi_var_entry *var)
 }
 
 efi_status_t __efi_runtime efi_var_mem_ins(
-				u16 *variable_name,
+				const u16 *variable_name,
 				const efi_guid_t *vendor, u32 attributes,
 				const efi_uintn_t size1, const void *data1,
 				const efi_uintn_t size2, const void *data2,
@@ -274,8 +274,9 @@ efi_status_t efi_var_mem_init(void)
 }
 
 efi_status_t __efi_runtime
-efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
-		     efi_uintn_t *data_size, void *data, u64 *timep)
+efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
+		     u32 *attributes, efi_uintn_t *data_size, void *data,
+		     u64 *timep)
 {
 	efi_uintn_t old_size;
 	struct efi_var_entry *var;
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index fa2b6bc7a8..5adc7f821a 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -45,7 +45,7 @@
  *
  * Return:	status code
  */
-static efi_status_t efi_variable_authenticate(u16 *variable,
+static efi_status_t efi_variable_authenticate(const u16 *variable,
 					      const efi_guid_t *vendor,
 					      efi_uintn_t *data_size,
 					      const void **data, u32 given_attr,
@@ -194,7 +194,7 @@ err:
 	return ret;
 }
 #else
-static efi_status_t efi_variable_authenticate(u16 *variable,
+static efi_status_t efi_variable_authenticate(const u16 *variable,
 					      const efi_guid_t *vendor,
 					      efi_uintn_t *data_size,
 					      const void **data, u32 given_attr,
@@ -205,7 +205,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
 #endif /* CONFIG_EFI_SECURE_BOOT */
 
 efi_status_t __efi_runtime
-efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
 		     u32 *attributes, efi_uintn_t *data_size, void *data,
 		     u64 *timep)
 {
@@ -219,7 +219,8 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
 	return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
 }
 
-efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+efi_status_t efi_set_variable_int(const u16 *variable_name,
+				  const efi_guid_t *vendor,
 				  u32 attributes, efi_uintn_t data_size,
 				  const void *data, bool ro_check)
 {
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index 51920bcb51..281f886124 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -284,7 +284,8 @@ out:
  * StMM can store internal attributes and properties for variables, i.e enabling
  * R/O variables
  */
-static efi_status_t set_property_int(u16 *variable_name, efi_uintn_t name_size,
+static efi_status_t set_property_int(const u16 *variable_name,
+				     efi_uintn_t name_size,
 				     const efi_guid_t *vendor,
 				     struct var_check_property *var_property)
 {
@@ -317,7 +318,8 @@ out:
 	return ret;
 }
 
-static efi_status_t get_property_int(u16 *variable_name, efi_uintn_t name_size,
+static efi_status_t get_property_int(const u16 *variable_name,
+				     efi_uintn_t name_size,
 				     const efi_guid_t *vendor,
 				     struct var_check_property *var_property)
 {
@@ -361,7 +363,8 @@ out:
 	return ret;
 }
 
-efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+efi_status_t efi_get_variable_int(const u16 *variable_name,
+				  const efi_guid_t *vendor,
 				  u32 *attributes, efi_uintn_t *data_size,
 				  void *data, u64 *timep)
 {
@@ -502,9 +505,10 @@ out:
 	return ret;
 }
 
-efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
-				  u32 attributes, efi_uintn_t data_size,
-				  const void *data, bool ro_check)
+efi_status_t efi_set_variable_int(const u16 *variable_name,
+				  const efi_guid_t *vendor, u32 attributes,
+				  efi_uintn_t data_size, const void *data,
+				  bool ro_check)
 {
 	efi_status_t ret, alt_ret = EFI_SUCCESS;
 	struct var_check_property var_property;
-- 
2.32.0

[PATCH v3 2/4] efi_loader: function to get GUID for variable name

[Heinrich Schuchardt]

In multiple places we need the default GUID matching a variable name.
The patch provides a library function. For secure boot related variables
like 'PK', 'KEK', 'db' a lookup table is used. For all other variable
names EFI_GLOBAL_VARIABLE is returned.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	no change
v2:
	adjust function documentation
---
 include/efi_variable.h          | 11 +++++++++++
 lib/efi_loader/efi_var_common.c |  9 +++++++++
 2 files changed, 20 insertions(+)

diff --git a/include/efi_variable.h b/include/efi_variable.h
index 8f666b2309..74909e8ec6 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -256,6 +256,17 @@ efi_status_t efi_init_secure_state(void);
 enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
 					     const efi_guid_t *guid);
 
+/**
+ * efi_auth_var_get_guid() - get the predefined GUID for a variable name
+ *
+ * For secure boot related variables a lookup table is used to determine
+ * the GUID. For all other variables EFI_GLOBAL_VARIABLE is returned.
+ *
+ * @name:	name of UEFI variable
+ * Return:	guid of UEFI variable
+ */
+const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
+
 /**
  * efi_get_next_variable_name_mem() - Runtime common code across efi variable
  *                                    implementations for GetNextVariable()
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index e179932124..3cbb7c96c2 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -385,6 +385,15 @@ enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
 	return EFI_AUTH_VAR_NONE;
 }
 
+const efi_guid_t *efi_auth_var_get_guid(const u16 *name)
+{
+	for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) {
+		if (!u16_strcmp(name, name_type[i].name))
+			return name_type[i].guid;
+	}
+	return &efi_global_variable_guid;
+}
+
 /**
  * efi_get_var() - read value of an EFI variable
  *
-- 
2.32.0

[PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt]

Simplify efi_sigstore_parse_sigdb() by using existing functions.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	Keep error handling in efi_sigstore_parse_sigdb()
v2:
	remove a superfluous check
---
 lib/efi_loader/efi_signature.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index bdd09881fc..97f6dfacd9 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
 	efi_uintn_t db_size;
 	efi_status_t ret;
 
-	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
-		vendor = &efi_global_variable_guid;
-	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
-		vendor = &efi_guid_image_security_database;
-	} else {
-		EFI_PRINT("unknown signature database, %ls\n", name);
-		return NULL;
-	}
+	vendor = efi_auth_var_get_guid(name);
 
 	/* retrieve variable data */
 	db_size = 0;
-	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
+	ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
 	if (ret == EFI_NOT_FOUND) {
 		EFI_PRINT("variable, %ls, not found\n", name);
 		sigstore = calloc(sizeof(*sigstore), 1);
-- 
2.32.0

[PATCH v3 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable()

[Heinrich Schuchardt]

Don't duplicate GUIDs.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v3:
	no change
v2:
	no change
---
 lib/efi_loader/efi_tcg2.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index ed1506012b..52bf1b775f 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -11,6 +11,7 @@
 #include <common.h>
 #include <dm.h>
 #include <efi_loader.h>
+#include <efi_variable.h>
 #include <efi_tcg2.h>
 #include <log.h>
 #include <malloc.h>
@@ -79,17 +80,12 @@ static const struct digest_info hash_algo_list[] = {
 	},
 };
 
-struct variable_info {
-	u16		*name;
-	const efi_guid_t	*guid;
-};
-
-static struct variable_info secure_variables[] = {
-	{L"SecureBoot", &efi_global_variable_guid},
-	{L"PK", &efi_global_variable_guid},
-	{L"KEK", &efi_global_variable_guid},
-	{L"db", &efi_guid_image_security_database},
-	{L"dbx", &efi_guid_image_security_database},
+static const u16 *secure_variables[] = {
+	u"SecureBoot",
+	u"PK",
+	u"KEK",
+	u"db",
+	u"dbx",
 };
 
 #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
@@ -1593,19 +1589,20 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
 
 	count = ARRAY_SIZE(secure_variables);
 	for (i = 0; i < count; i++) {
+		const efi_guid_t *guid;
+
+		guid = efi_auth_var_get_guid(secure_variables[i]);
+
 		/*
 		 * According to the TCG2 PC Client PFP spec, "SecureBoot",
 		 * "PK", "KEK", "db" and "dbx" variables must be measured
 		 * even if they are empty.
 		 */
-		data = efi_get_var(secure_variables[i].name,
-				   secure_variables[i].guid,
-				   &data_size);
+		data = efi_get_var(secure_variables[i], guid, &data_size);
 
 		ret = tcg2_measure_variable(dev, 7,
 					    EV_EFI_VARIABLE_DRIVER_CONFIG,
-					    secure_variables[i].name,
-					    secure_variables[i].guid,
+					    secure_variables[i], guid,
 					    data_size, data);
 		free(data);
 		if (ret != EFI_SUCCESS)
-- 
2.32.0

Re: [PATCH v3 2/4] efi_loader: function to get GUID for variable name

[Ilias Apalodimas]

On Sun, Oct 03, 2021 at 11:23:18AM +0200, Heinrich Schuchardt wrote:
> In multiple places we need the default GUID matching a variable name.
> The patch provides a library function. For secure boot related variables
> like 'PK', 'KEK', 'db' a lookup table is used. For all other variable
> names EFI_GLOBAL_VARIABLE is returned.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	no change
> v2:
> 	adjust function documentation
> ---
>  include/efi_variable.h          | 11 +++++++++++
>  lib/efi_loader/efi_var_common.c |  9 +++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 8f666b2309..74909e8ec6 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -256,6 +256,17 @@ efi_status_t efi_init_secure_state(void);
>  enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
>  					     const efi_guid_t *guid);
>  
> +/**
> + * efi_auth_var_get_guid() - get the predefined GUID for a variable name
> + *
> + * For secure boot related variables a lookup table is used to determine
> + * the GUID. For all other variables EFI_GLOBAL_VARIABLE is returned.
> + *
> + * @name:	name of UEFI variable
> + * Return:	guid of UEFI variable
> + */
> +const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
> +
>  /**
>   * efi_get_next_variable_name_mem() - Runtime common code across efi variable
>   *                                    implementations for GetNextVariable()
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index e179932124..3cbb7c96c2 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -385,6 +385,15 @@ enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
>  	return EFI_AUTH_VAR_NONE;
>  }
>  
> +const efi_guid_t *efi_auth_var_get_guid(const u16 *name)
> +{
> +	for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) {
> +		if (!u16_strcmp(name, name_type[i].name))
> +			return name_type[i].guid;
> +	}
> +	return &efi_global_variable_guid;
> +}
> +
>  /**
>   * efi_get_var() - read value of an EFI variable
>   *
> -- 
> 2.32.0
> 

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v3 4/4] efi_loader: simplify tcg2_measure_secure_boot_variable()

[Ilias Apalodimas]

On Sun, Oct 03, 2021 at 11:23:20AM +0200, Heinrich Schuchardt wrote:
> Don't duplicate GUIDs.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	no change
> v2:
> 	no change
> ---
>  lib/efi_loader/efi_tcg2.c | 29 +++++++++++++----------------
>  1 file changed, 13 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index ed1506012b..52bf1b775f 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -11,6 +11,7 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <efi_loader.h>
> +#include <efi_variable.h>
>  #include <efi_tcg2.h>
>  #include <log.h>
>  #include <malloc.h>
> @@ -79,17 +80,12 @@ static const struct digest_info hash_algo_list[] = {
>  	},
>  };
>  
> -struct variable_info {
> -	u16		*name;
> -	const efi_guid_t	*guid;
> -};
> -
> -static struct variable_info secure_variables[] = {
> -	{L"SecureBoot", &efi_global_variable_guid},
> -	{L"PK", &efi_global_variable_guid},
> -	{L"KEK", &efi_global_variable_guid},
> -	{L"db", &efi_guid_image_security_database},
> -	{L"dbx", &efi_guid_image_security_database},
> +static const u16 *secure_variables[] = {
> +	u"SecureBoot",
> +	u"PK",
> +	u"KEK",
> +	u"db",
> +	u"dbx",
>  };
>  
>  #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
> @@ -1593,19 +1589,20 @@ static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
>  
>  	count = ARRAY_SIZE(secure_variables);
>  	for (i = 0; i < count; i++) {
> +		const efi_guid_t *guid;
> +
> +		guid = efi_auth_var_get_guid(secure_variables[i]);
> +
>  		/*
>  		 * According to the TCG2 PC Client PFP spec, "SecureBoot",
>  		 * "PK", "KEK", "db" and "dbx" variables must be measured
>  		 * even if they are empty.
>  		 */
> -		data = efi_get_var(secure_variables[i].name,
> -				   secure_variables[i].guid,
> -				   &data_size);
> +		data = efi_get_var(secure_variables[i], guid, &data_size);
>  
>  		ret = tcg2_measure_variable(dev, 7,
>  					    EV_EFI_VARIABLE_DRIVER_CONFIG,
> -					    secure_variables[i].name,
> -					    secure_variables[i].guid,
> +					    secure_variables[i], guid,
>  					    data_size, data);
>  		free(data);
>  		if (ret != EFI_SUCCESS)
> -- 
> 2.32.0
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v3 1/4] efi_loader: treat UEFI variable name as const

[Ilias Apalodimas]

On Sun, Oct 03, 2021 at 11:23:17AM +0200, Heinrich Schuchardt wrote:
> UEFI variable names are typically constants and hence should be defined as
> const. Unfortunately some of our API functions do not define the parameters
> for UEFI variable names as const. This requires unnecessary conversions.
> 
> Adjust parameters of several internal functions to tre UEFI variable names
> as const.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	no change
> v2:
> 	adjust commit message
> ---
>  include/efi_loader.h              |  2 +-
>  include/efi_variable.h            | 16 ++++++++++------
>  lib/efi_loader/efi_tcg2.c         |  2 +-
>  lib/efi_loader/efi_var_common.c   |  5 +++--
>  lib/efi_loader/efi_var_mem.c      |  7 ++++---
>  lib/efi_loader/efi_variable.c     |  9 +++++----
>  lib/efi_loader/efi_variable_tee.c | 16 ++++++++++------
>  7 files changed, 34 insertions(+), 23 deletions(-)
> 
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index c440962fe5..125052d002 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -816,7 +816,7 @@ efi_status_t EFIAPI efi_query_variable_info(
>  			u64 *remaining_variable_storage_size,
>  			u64 *maximum_variable_size);
>  
> -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
> +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size);
>  
>  /*
>   * See section 3.1.3 in the v2.7 UEFI spec for more details on
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 0440d356bc..8f666b2309 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -32,7 +32,8 @@ enum efi_auth_var_type {
>   * @timep:		authentication time (seconds since start of epoch)
>   * Return:		status code
>   */
> -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> +efi_status_t efi_get_variable_int(const u16 *variable_name,
> +				  const efi_guid_t *vendor,
>  				  u32 *attributes, efi_uintn_t *data_size,
>  				  void *data, u64 *timep);
>  
> @@ -47,7 +48,8 @@ efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
>   * @ro_check:		check the read only read only bit in attributes
>   * Return:		status code
>   */
> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> +efi_status_t efi_set_variable_int(const u16 *variable_name,
> +				  const efi_guid_t *vendor,
>  				  u32 attributes, efi_uintn_t data_size,
>  				  const void *data, bool ro_check);
>  
> @@ -224,7 +226,7 @@ void efi_var_mem_del(struct efi_var_entry *var);
>   * @time:		time of authentication (as seconds since start of epoch)
>   * Result:		status code
>   */
> -efi_status_t efi_var_mem_ins(u16 *variable_name,
> +efi_status_t efi_var_mem_ins(const u16 *variable_name,
>  			     const efi_guid_t *vendor, u32 attributes,
>  			     const efi_uintn_t size1, const void *data1,
>  			     const efi_uintn_t size2, const void *data2,
> @@ -251,7 +253,8 @@ efi_status_t efi_init_secure_state(void);
>   * @guid:	guid of UEFI variable
>   * Return:	identifier for authentication related variables
>   */
> -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid);
> +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
> +					     const efi_guid_t *guid);
>  
>  /**
>   * efi_get_next_variable_name_mem() - Runtime common code across efi variable
> @@ -280,8 +283,9 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
>   * Return:		status code
>   */
>  efi_status_t __efi_runtime
> -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
> -		     efi_uintn_t *data_size, void *data, u64 *timep);
> +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> +		     u32 *attributes, efi_uintn_t *data_size, void *data,
> +		     u64 *timep);
>  
>  /**
>   * efi_get_variable_runtime() - runtime implementation of GetVariable()
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index d3b8f93f14..ed1506012b 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -1365,7 +1365,7 @@ static efi_status_t efi_append_scrtm_version(struct udevice *dev)
>   * Return:	status code
>   */
>  static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> -					  u32 event_type, u16 *var_name,
> +					  u32 event_type, const u16 *var_name,
>  					  const efi_guid_t *guid,
>  					  efi_uintn_t data_size, u8 *data)
>  {
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index a00bbf1620..e179932124 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -374,7 +374,8 @@ bool efi_secure_boot_enabled(void)
>  	return efi_secure_boot;
>  }
>  
> -enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid)
> +enum efi_auth_var_type efi_auth_var_get_type(const u16 *name,
> +					     const efi_guid_t *guid)
>  {
>  	for (size_t i = 0; i < ARRAY_SIZE(name_type); ++i) {
>  		if (!u16_strcmp(name, name_type[i].name) &&
> @@ -393,7 +394,7 @@ enum efi_auth_var_type efi_auth_var_get_type(u16 *name, const efi_guid_t *guid)
>   *
>   * Return:	buffer with variable data or NULL
>   */
> -void *efi_get_var(u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
> +void *efi_get_var(const u16 *name, const efi_guid_t *vendor, efi_uintn_t *size)
>  {
>  	efi_status_t ret;
>  	void *buf = NULL;
> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> index 3d335a8274..13909b1d26 100644
> --- a/lib/efi_loader/efi_var_mem.c
> +++ b/lib/efi_loader/efi_var_mem.c
> @@ -134,7 +134,7 @@ void __efi_runtime efi_var_mem_del(struct efi_var_entry *var)
>  }
>  
>  efi_status_t __efi_runtime efi_var_mem_ins(
> -				u16 *variable_name,
> +				const u16 *variable_name,
>  				const efi_guid_t *vendor, u32 attributes,
>  				const efi_uintn_t size1, const void *data1,
>  				const efi_uintn_t size2, const void *data2,
> @@ -274,8 +274,9 @@ efi_status_t efi_var_mem_init(void)
>  }
>  
>  efi_status_t __efi_runtime
> -efi_get_variable_mem(u16 *variable_name, const efi_guid_t *vendor, u32 *attributes,
> -		     efi_uintn_t *data_size, void *data, u64 *timep)
> +efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> +		     u32 *attributes, efi_uintn_t *data_size, void *data,
> +		     u64 *timep)
>  {
>  	efi_uintn_t old_size;
>  	struct efi_var_entry *var;
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index fa2b6bc7a8..5adc7f821a 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -45,7 +45,7 @@
>   *
>   * Return:	status code
>   */
> -static efi_status_t efi_variable_authenticate(u16 *variable,
> +static efi_status_t efi_variable_authenticate(const u16 *variable,
>  					      const efi_guid_t *vendor,
>  					      efi_uintn_t *data_size,
>  					      const void **data, u32 given_attr,
> @@ -194,7 +194,7 @@ err:
>  	return ret;
>  }
>  #else
> -static efi_status_t efi_variable_authenticate(u16 *variable,
> +static efi_status_t efi_variable_authenticate(const u16 *variable,
>  					      const efi_guid_t *vendor,
>  					      efi_uintn_t *data_size,
>  					      const void **data, u32 given_attr,
> @@ -205,7 +205,7 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
>  #endif /* CONFIG_EFI_SECURE_BOOT */
>  
>  efi_status_t __efi_runtime
> -efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> +efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
>  		     u32 *attributes, efi_uintn_t *data_size, void *data,
>  		     u64 *timep)
>  {
> @@ -219,7 +219,8 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
>  	return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
>  }
>  
> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> +efi_status_t efi_set_variable_int(const u16 *variable_name,
> +				  const efi_guid_t *vendor,
>  				  u32 attributes, efi_uintn_t data_size,
>  				  const void *data, bool ro_check)
>  {
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index 51920bcb51..281f886124 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -284,7 +284,8 @@ out:
>   * StMM can store internal attributes and properties for variables, i.e enabling
>   * R/O variables
>   */
> -static efi_status_t set_property_int(u16 *variable_name, efi_uintn_t name_size,
> +static efi_status_t set_property_int(const u16 *variable_name,
> +				     efi_uintn_t name_size,
>  				     const efi_guid_t *vendor,
>  				     struct var_check_property *var_property)
>  {
> @@ -317,7 +318,8 @@ out:
>  	return ret;
>  }
>  
> -static efi_status_t get_property_int(u16 *variable_name, efi_uintn_t name_size,
> +static efi_status_t get_property_int(const u16 *variable_name,
> +				     efi_uintn_t name_size,
>  				     const efi_guid_t *vendor,
>  				     struct var_check_property *var_property)
>  {
> @@ -361,7 +363,8 @@ out:
>  	return ret;
>  }
>  
> -efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> +efi_status_t efi_get_variable_int(const u16 *variable_name,
> +				  const efi_guid_t *vendor,
>  				  u32 *attributes, efi_uintn_t *data_size,
>  				  void *data, u64 *timep)
>  {
> @@ -502,9 +505,10 @@ out:
>  	return ret;
>  }
>  
> -efi_status_t efi_set_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> -				  u32 attributes, efi_uintn_t data_size,
> -				  const void *data, bool ro_check)
> +efi_status_t efi_set_variable_int(const u16 *variable_name,
> +				  const efi_guid_t *vendor, u32 attributes,
> +				  efi_uintn_t data_size, const void *data,
> +				  bool ro_check)
>  {
>  	efi_status_t ret, alt_ret = EFI_SUCCESS;
>  	struct var_check_property var_property;
> -- 
> 2.32.0
> 
Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Ilias Apalodimas]

On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v3:
> 	Keep error handling in efi_sigstore_parse_sigdb()
> v2:
> 	remove a superfluous check
> ---
>  lib/efi_loader/efi_signature.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index bdd09881fc..97f6dfacd9 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>  	efi_uintn_t db_size;
>  	efi_status_t ret;
>  
> -	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> -		vendor = &efi_global_variable_guid;
> -	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> -		vendor = &efi_guid_image_security_database;
> -	} else {
> -		EFI_PRINT("unknown signature database, %ls\n", name);
> -		return NULL;
> -	}
> +	vendor = efi_auth_var_get_guid(name);

Should we return NULL if we get back the default guid?

>  
>  	/* retrieve variable data */
>  	db_size = 0;
> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> +	ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>  	if (ret == EFI_NOT_FOUND) {
>  		EFI_PRINT("variable, %ls, not found\n", name);
>  		sigstore = calloc(sizeof(*sigstore), 1);
> -- 
> 2.32.0
> 

Regards
/Ilias

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt]

On 10/6/21 08:29, Ilias Apalodimas wrote:
> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v3:
>> 	Keep error handling in efi_sigstore_parse_sigdb()
>> v2:
>> 	remove a superfluous check
>> ---
>>   lib/efi_loader/efi_signature.c | 11 ++---------
>>   1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>> index bdd09881fc..97f6dfacd9 100644
>> --- a/lib/efi_loader/efi_signature.c
>> +++ b/lib/efi_loader/efi_signature.c
>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>   	efi_uintn_t db_size;
>>   	efi_status_t ret;
>>   
>> -	if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>> -		vendor = &efi_global_variable_guid;
>> -	} else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>> -		vendor = &efi_guid_image_security_database;
>> -	} else {
>> -		EFI_PRINT("unknown signature database, %ls\n", name);
>> -		return NULL;
>> -	}
>> +	vendor = efi_auth_var_get_guid(name);
> 
> Should we return NULL if we get back the default guid?

efi_sigstore_parse_sigdb() is only called with fixed values of 'name'. 
So how should this occur?

Best regards

Heinrich

> 
>>   
>>   	/* retrieve variable data */
>>   	db_size = 0;
>> -	ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>> +	ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>>   	if (ret == EFI_NOT_FOUND) {
>>   		EFI_PRINT("variable, %ls, not found\n", name);
>>   		sigstore = calloc(sizeof(*sigstore), 1);
>> -- 
>> 2.32.0
>>
> 
> Regards
> /Ilias
> 

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Ilias Apalodimas]

On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/6/21 08:29, Ilias Apalodimas wrote:
> > On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
> >> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> >>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >> v3:
> >>      Keep error handling in efi_sigstore_parse_sigdb()
> >> v2:
> >>      remove a superfluous check
> >> ---
> >>   lib/efi_loader/efi_signature.c | 11 ++---------
> >>   1 file changed, 2 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> >> index bdd09881fc..97f6dfacd9 100644
> >> --- a/lib/efi_loader/efi_signature.c
> >> +++ b/lib/efi_loader/efi_signature.c
> >> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
> >>      efi_uintn_t db_size;
> >>      efi_status_t ret;
> >>
> >> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> >> -            vendor = &efi_global_variable_guid;
> >> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> >> -            vendor = &efi_guid_image_security_database;
> >> -    } else {
> >> -            EFI_PRINT("unknown signature database, %ls\n", name);
> >> -            return NULL;
> >> -    }
> >> +    vendor = efi_auth_var_get_guid(name);
> >
> > Should we return NULL if we get back the default guid?
>
> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
> So how should this occur?

Bugs that slip through maybe?  I generally prefer being more pedantic
with security related code

Regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> >>
> >>      /* retrieve variable data */
> >>      db_size = 0;
> >> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> >> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
> >>      if (ret == EFI_NOT_FOUND) {
> >>              EFI_PRINT("variable, %ls, not found\n", name);
> >>              sigstore = calloc(sizeof(*sigstore), 1);
> >> --
> >> 2.32.0
> >>
> >
> > Regards
> > /Ilias
> >

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt]

On 10/6/21 10:02, Ilias Apalodimas wrote:
> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 10/6/21 08:29, Ilias Apalodimas wrote:
>>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
>>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v3:
>>>>       Keep error handling in efi_sigstore_parse_sigdb()
>>>> v2:
>>>>       remove a superfluous check
>>>> ---
>>>>    lib/efi_loader/efi_signature.c | 11 ++---------
>>>>    1 file changed, 2 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>>>> index bdd09881fc..97f6dfacd9 100644
>>>> --- a/lib/efi_loader/efi_signature.c
>>>> +++ b/lib/efi_loader/efi_signature.c
>>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>>>       efi_uintn_t db_size;
>>>>       efi_status_t ret;
>>>>
>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>>>> -            vendor = &efi_global_variable_guid;
>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>>>> -            vendor = &efi_guid_image_security_database;
>>>> -    } else {
>>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
>>>> -            return NULL;
>>>> -    }
>>>> +    vendor = efi_auth_var_get_guid(name);
>>>
>>> Should we return NULL if we get back the default guid?
>>
>> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
>> So how should this occur?
> 
> Bugs that slip through maybe?  I generally prefer being more pedantic
> with security related code

PK and KEK use efi_global_variable_guid and will be used as argument for 
efi_sigstore_parse_sigdb(). You proposed check would break the code.

Best regards

Heinrich

> 
> Regards
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>>       /* retrieve variable data */
>>>>       db_size = 0;
>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>>>>       if (ret == EFI_NOT_FOUND) {
>>>>               EFI_PRINT("variable, %ls, not found\n", name);
>>>>               sigstore = calloc(sizeof(*sigstore), 1);
>>>> --
>>>> 2.32.0
>>>>
>>>
>>> Regards
>>> /Ilias
>>>

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt]

On 10/6/21 10:02, Ilias Apalodimas wrote:
> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>>
>>
>> On 10/6/21 08:29, Ilias Apalodimas wrote:
>>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
>>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>>>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v3:
>>>>       Keep error handling in efi_sigstore_parse_sigdb()
>>>> v2:
>>>>       remove a superfluous check
>>>> ---
>>>>    lib/efi_loader/efi_signature.c | 11 ++---------
>>>>    1 file changed, 2 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>>>> index bdd09881fc..97f6dfacd9 100644
>>>> --- a/lib/efi_loader/efi_signature.c
>>>> +++ b/lib/efi_loader/efi_signature.c
>>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>>>       efi_uintn_t db_size;
>>>>       efi_status_t ret;
>>>>
>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>>>> -            vendor = &efi_global_variable_guid;
>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>>>> -            vendor = &efi_guid_image_security_database;
>>>> -    } else {
>>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
>>>> -            return NULL;
>>>> -    }
>>>> +    vendor = efi_auth_var_get_guid(name);
>>>
>>> Should we return NULL if we get back the default guid?
>>
>> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
>> So how should this occur?
> 
> Bugs that slip through maybe?  I generally prefer being more pedantic
> with security related code

PK and KEK use efi_global_variable_guid and will be used as argument for 
efi_sigstore_parse_sigdb(). Your proposed check would break the code.

Best regards

Heinrich

> 
> Regards
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>>
>>>>       /* retrieve variable data */
>>>>       db_size = 0;
>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>>>>       if (ret == EFI_NOT_FOUND) {
>>>>               EFI_PRINT("variable, %ls, not found\n", name);
>>>>               sigstore = calloc(sizeof(*sigstore), 1);
>>>> --
>>>> 2.32.0
>>>>
>>>
>>> Regards
>>> /Ilias
>>>

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Ilias Apalodimas]

On Wed, 6 Oct 2021 at 16:15, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 10/6/21 10:02, Ilias Apalodimas wrote:
> > On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >>
> >>
> >> On 10/6/21 08:29, Ilias Apalodimas wrote:
> >>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
> >>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> >>>>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>> v3:
> >>>>       Keep error handling in efi_sigstore_parse_sigdb()
> >>>> v2:
> >>>>       remove a superfluous check
> >>>> ---
> >>>>    lib/efi_loader/efi_signature.c | 11 ++---------
> >>>>    1 file changed, 2 insertions(+), 9 deletions(-)
> >>>>
> >>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> >>>> index bdd09881fc..97f6dfacd9 100644
> >>>> --- a/lib/efi_loader/efi_signature.c
> >>>> +++ b/lib/efi_loader/efi_signature.c
> >>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
> >>>>       efi_uintn_t db_size;
> >>>>       efi_status_t ret;
> >>>>
> >>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> >>>> -            vendor = &efi_global_variable_guid;
> >>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> >>>> -            vendor = &efi_guid_image_security_database;
> >>>> -    } else {
> >>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
> >>>> -            return NULL;
> >>>> -    }
> >>>> +    vendor = efi_auth_var_get_guid(name);
> >>>
> >>> Should we return NULL if we get back the default guid?
> >>
> >> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
> >> So how should this occur?
> >
> > Bugs that slip through maybe?  I generally prefer being more pedantic
> > with security related code
>
> PK and KEK use efi_global_variable_guid and will be used as argument for
> efi_sigstore_parse_sigdb(). Your proposed check would break the code.

Yea that's the reason I was asking about efi_auth_var_get_guid().  I
would prefer if that returned NULL if the variable is not found
instead of the default GUID

Regards
/Ilias
>
> Best regards
>
> Heinrich
>
> >
> > Regards
> > /Ilias
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>>>
> >>>>       /* retrieve variable data */
> >>>>       db_size = 0;
> >>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> >>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
> >>>>       if (ret == EFI_NOT_FOUND) {
> >>>>               EFI_PRINT("variable, %ls, not found\n", name);
> >>>>               sigstore = calloc(sizeof(*sigstore), 1);
> >>>> --
> >>>> 2.32.0
> >>>>
> >>>
> >>> Regards
> >>> /Ilias
> >>>
>

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Heinrich Schuchardt]

On 10/6/21 16:05, Ilias Apalodimas wrote:
> On Wed, 6 Oct 2021 at 16:15, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 10/6/21 10:02, Ilias Apalodimas wrote:
>>> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>>
>>>>
>>>> On 10/6/21 08:29, Ilias Apalodimas wrote:
>>>>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
>>>>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
>>>>>>
>>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>>> ---
>>>>>> v3:
>>>>>>        Keep error handling in efi_sigstore_parse_sigdb()
>>>>>> v2:
>>>>>>        remove a superfluous check
>>>>>> ---
>>>>>>     lib/efi_loader/efi_signature.c | 11 ++---------
>>>>>>     1 file changed, 2 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
>>>>>> index bdd09881fc..97f6dfacd9 100644
>>>>>> --- a/lib/efi_loader/efi_signature.c
>>>>>> +++ b/lib/efi_loader/efi_signature.c
>>>>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
>>>>>>        efi_uintn_t db_size;
>>>>>>        efi_status_t ret;
>>>>>>
>>>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
>>>>>> -            vendor = &efi_global_variable_guid;
>>>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
>>>>>> -            vendor = &efi_guid_image_security_database;
>>>>>> -    } else {
>>>>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
>>>>>> -            return NULL;
>>>>>> -    }
>>>>>> +    vendor = efi_auth_var_get_guid(name);
>>>>>
>>>>> Should we return NULL if we get back the default guid?
>>>>
>>>> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
>>>> So how should this occur?
>>>
>>> Bugs that slip through maybe?  I generally prefer being more pedantic
>>> with security related code
>>
>> PK and KEK use efi_global_variable_guid and will be used as argument for
>> efi_sigstore_parse_sigdb(). Your proposed check would break the code.
> 
> Yea that's the reason I was asking about efi_auth_var_get_guid().  I
> would prefer if that returned NULL if the variable is not found
> instead of the default GUID

As discussed the functions is only called for hardcoded arguments.

Function efi_auth_var_get_guid() parses a list contains dbx, AuditMode, 
DeployedMode. So the check that you propose cannot safeguard against a 
developer misusing efi_sigstore_parse_sigdb().

Best regards

Heinrich

> 
> Regards
> /Ilias
>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>> Regards
>>> /Ilias
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>
>>>>>>
>>>>>>        /* retrieve variable data */
>>>>>>        db_size = 0;
>>>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
>>>>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
>>>>>>        if (ret == EFI_NOT_FOUND) {
>>>>>>                EFI_PRINT("variable, %ls, not found\n", name);
>>>>>>                sigstore = calloc(sizeof(*sigstore), 1);
>>>>>> --
>>>>>> 2.32.0
>>>>>>
>>>>>
>>>>> Regards
>>>>> /Ilias
>>>>>
>>

Re: [PATCH v3 3/4] efi_loader: simplify efi_sigstore_parse_sigdb()

[Ilias Apalodimas]

On Thu, 7 Oct 2021 at 08:54, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
>
>
> On 10/6/21 16:05, Ilias Apalodimas wrote:
> > On Wed, 6 Oct 2021 at 16:15, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 10/6/21 10:02, Ilias Apalodimas wrote:
> >>> On Wed, 6 Oct 2021 at 10:21, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 10/6/21 08:29, Ilias Apalodimas wrote:
> >>>>> On Sun, Oct 03, 2021 at 11:23:19AM +0200, Heinrich Schuchardt wrote:
> >>>>>> Simplify efi_sigstore_parse_sigdb() by using existing functions.
> >>>>>>
> >>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>>>> ---
> >>>>>> v3:
> >>>>>>        Keep error handling in efi_sigstore_parse_sigdb()
> >>>>>> v2:
> >>>>>>        remove a superfluous check
> >>>>>> ---
> >>>>>>     lib/efi_loader/efi_signature.c | 11 ++---------
> >>>>>>     1 file changed, 2 insertions(+), 9 deletions(-)
> >>>>>>
> >>>>>> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> >>>>>> index bdd09881fc..97f6dfacd9 100644
> >>>>>> --- a/lib/efi_loader/efi_signature.c
> >>>>>> +++ b/lib/efi_loader/efi_signature.c
> >>>>>> @@ -746,18 +746,11 @@ struct efi_signature_store *efi_sigstore_parse_sigdb(u16 *name)
> >>>>>>        efi_uintn_t db_size;
> >>>>>>        efi_status_t ret;
> >>>>>>
> >>>>>> -    if (!u16_strcmp(name, L"PK") || !u16_strcmp(name, L"KEK")) {
> >>>>>> -            vendor = &efi_global_variable_guid;
> >>>>>> -    } else if (!u16_strcmp(name, L"db") || !u16_strcmp(name, L"dbx")) {
> >>>>>> -            vendor = &efi_guid_image_security_database;
> >>>>>> -    } else {
> >>>>>> -            EFI_PRINT("unknown signature database, %ls\n", name);
> >>>>>> -            return NULL;
> >>>>>> -    }
> >>>>>> +    vendor = efi_auth_var_get_guid(name);
> >>>>>
> >>>>> Should we return NULL if we get back the default guid?
> >>>>
> >>>> efi_sigstore_parse_sigdb() is only called with fixed values of 'name'.
> >>>> So how should this occur?
> >>>
> >>> Bugs that slip through maybe?  I generally prefer being more pedantic
> >>> with security related code
> >>
> >> PK and KEK use efi_global_variable_guid and will be used as argument for
> >> efi_sigstore_parse_sigdb(). Your proposed check would break the code.
> >
> > Yea that's the reason I was asking about efi_auth_var_get_guid().  I
> > would prefer if that returned NULL if the variable is not found
> > instead of the default GUID
>
> As discussed the functions is only called for hardcoded arguments.
>
> Function efi_auth_var_get_guid() parses a list contains dbx, AuditMode,
> DeployedMode. So the check that you propose cannot safeguard against a
> developer misusing efi_sigstore_parse_sigdb().

Yea fair enough, I was trying to think if it would make sense to
re-use it for other variables.


>
> Best regards
>
> Heinrich
>
> >
> > Regards
> > /Ilias
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>>
> >>> Regards
> >>> /Ilias
> >>>>
> >>>> Best regards
> >>>>
> >>>> Heinrich
> >>>>
> >>>>>
> >>>>>>
> >>>>>>        /* retrieve variable data */
> >>>>>>        db_size = 0;
> >>>>>> -    ret = EFI_CALL(efi_get_variable(name, vendor, NULL, &db_size, NULL));
> >>>>>> +    ret = efi_get_variable_int(name, vendor, NULL, &db_size, NULL);
> >>>>>>        if (ret == EFI_NOT_FOUND) {
> >>>>>>                EFI_PRINT("variable, %ls, not found\n", name);
> >>>>>>                sigstore = calloc(sizeof(*sigstore), 1);
> >>>>>> --
> >>>>>> 2.32.0
> >>>>>>
> >>>>>
> >>>>> Regards
> >>>>> /Ilias
> >>>>>
> >>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>