* [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
@ 2020-06-22 16:10 Heinrich Schuchardt
2020-06-22 23:44 ` AKASHI Takahiro
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2020-06-22 16:10 UTC (permalink / raw)
To: u-boot
We currently have two implementations of UEFI variables:
* variables provided via an OP-TEE module
* variables stored in the U-Boot environment
Read only variables are up to now only implemented in the U-Boot
environment implementation.
Provide a common interface for both implementations that allows handling
read-only variables.
Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
v2:
add missing efi_variable.h
consider attributes==NULL in efi_variable_get()
---
include/efi_variable.h | 40 +++++++
lib/efi_loader/Makefile | 1 +
lib/efi_loader/efi_variable.c | 171 ++++++++-------------------
lib/efi_loader/efi_variable_common.c | 79 +++++++++++++
lib/efi_loader/efi_variable_tee.c | 75 ++++--------
5 files changed, 188 insertions(+), 178 deletions(-)
create mode 100644 include/efi_variable.h
create mode 100644 lib/efi_loader/efi_variable_common.c
diff --git a/include/efi_variable.h b/include/efi_variable.h
new file mode 100644
index 0000000000..784dbd9cf4
--- /dev/null
+++ b/include/efi_variable.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#ifndef _EFI_VARIABLE_H
+#define _EFI_VARIABLE_H
+
+#define EFI_VARIABLE_READ_ONLY BIT(31)
+
+/**
+ * efi_get_variable() - retrieve value of a UEFI variable
+ *
+ * @variable_name: name of the variable
+ * @vendor: vendor GUID
+ * @attributes: attributes of the variable
+ * @data_size: size of the buffer to which the variable value is copied
+ * @data: buffer to which the variable value is copied
+ * Return: status code
+ */
+efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+ u32 *attributes, efi_uintn_t *data_size,
+ void *data);
+
+/**
+ * efi_set_variable() - set value of a UEFI variable
+ *
+ * @variable_name: name of the variable
+ * @vendor: vendor GUID
+ * @attributes: attributes of the variable
+ * @data_size: size of the buffer with the variable value
+ * @data: buffer with the variable value
+ * @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,
+ u32 attributes, efi_uintn_t data_size,
+ const void *data, bool ro_check);
+
+#endif
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 57c7e66ea0..16b93ef7f0 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -35,6 +35,7 @@ obj-y += efi_root_node.o
obj-y += efi_runtime.o
obj-y += efi_setup.o
obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
+obj-y += efi_variable_common.o
ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
obj-y += efi_variable_tee.o
else
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index e097670e28..85df1427bc 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -7,6 +7,7 @@
#include <common.h>
#include <efi_loader.h>
+#include <efi_variable.h>
#include <env.h>
#include <env_internal.h>
#include <hexdump.h>
@@ -15,7 +16,6 @@
#include <search.h>
#include <uuid.h>
#include <crypto/pkcs7_parser.h>
-#include <linux/bitops.h>
#include <linux/compat.h>
#include <u-boot/crc.h>
@@ -30,20 +30,6 @@ static bool efi_secure_boot;
static int efi_secure_mode;
static u8 efi_vendor_keys;
-#define READ_ONLY BIT(31)
-
-static efi_status_t efi_get_variable_common(u16 *variable_name,
- const efi_guid_t *vendor,
- u32 *attributes,
- efi_uintn_t *data_size, void *data);
-
-static efi_status_t efi_set_variable_common(u16 *variable_name,
- const efi_guid_t *vendor,
- u32 attributes,
- efi_uintn_t data_size,
- const void *data,
- bool ro_check);
-
/*
* Mapping between EFI variables and u-boot variables:
*
@@ -154,7 +140,7 @@ static const char *parse_attr(const char *str, u32 *attrp, u64 *timep)
str++;
if ((s = prefix(str, "ro"))) {
- attr |= READ_ONLY;
+ attr |= EFI_VARIABLE_READ_ONLY;
} else if ((s = prefix(str, "nv"))) {
attr |= EFI_VARIABLE_NON_VOLATILE;
} else if ((s = prefix(str, "boot"))) {
@@ -202,29 +188,29 @@ static efi_status_t efi_set_secure_state(int sec_boot, int setup_mode,
attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
EFI_VARIABLE_RUNTIME_ACCESS |
- READ_ONLY;
- ret = efi_set_variable_common(L"SecureBoot", &efi_global_variable_guid,
- attributes, sizeof(sec_boot), &sec_boot,
- false);
+ EFI_VARIABLE_READ_ONLY;
+ ret = efi_set_variable_int(L"SecureBoot", &efi_global_variable_guid,
+ attributes, sizeof(sec_boot), &sec_boot,
+ false);
if (ret != EFI_SUCCESS)
goto err;
- ret = efi_set_variable_common(L"SetupMode", &efi_global_variable_guid,
- attributes, sizeof(setup_mode),
- &setup_mode, false);
+ ret = efi_set_variable_int(L"SetupMode", &efi_global_variable_guid,
+ attributes, sizeof(setup_mode),
+ &setup_mode, false);
if (ret != EFI_SUCCESS)
goto err;
- ret = efi_set_variable_common(L"AuditMode", &efi_global_variable_guid,
- attributes, sizeof(audit_mode),
- &audit_mode, false);
+ ret = efi_set_variable_int(L"AuditMode", &efi_global_variable_guid,
+ attributes, sizeof(audit_mode),
+ &audit_mode, false);
if (ret != EFI_SUCCESS)
goto err;
- ret = efi_set_variable_common(L"DeployedMode",
- &efi_global_variable_guid, attributes,
- sizeof(deployed_mode), &deployed_mode,
- false);
+ ret = efi_set_variable_int(L"DeployedMode",
+ &efi_global_variable_guid, attributes,
+ sizeof(deployed_mode), &deployed_mode,
+ false);
err:
return ret;
}
@@ -234,7 +220,7 @@ err:
* @mode: new state
*
* Depending on @mode, secure boot related variables are updated.
- * Those variables are *read-only* for users, efi_set_variable_common()
+ * Those variables are *read-only* for users, efi_set_variable_int()
* is called here.
*
* Return: status code
@@ -252,10 +238,10 @@ static efi_status_t efi_transfer_secure_state(enum efi_secure_mode mode)
efi_secure_boot = true;
} else if (mode == EFI_MODE_AUDIT) {
- ret = efi_set_variable_common(L"PK", &efi_global_variable_guid,
- EFI_VARIABLE_BOOTSERVICE_ACCESS |
- EFI_VARIABLE_RUNTIME_ACCESS,
- 0, NULL, false);
+ 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_SUCCESS)
goto err;
@@ -307,8 +293,8 @@ static efi_status_t efi_init_secure_state(void)
*/
size = 0;
- ret = efi_get_variable_common(L"PK", &efi_global_variable_guid,
- NULL, &size, NULL);
+ ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
+ NULL, &size, NULL);
if (ret == EFI_BUFFER_TOO_SMALL) {
if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT))
mode = EFI_MODE_USER;
@@ -325,13 +311,13 @@ static efi_status_t efi_init_secure_state(void)
ret = efi_transfer_secure_state(mode);
if (ret == EFI_SUCCESS)
- ret = efi_set_variable_common(L"VendorKeys",
- &efi_global_variable_guid,
- EFI_VARIABLE_BOOTSERVICE_ACCESS |
- EFI_VARIABLE_RUNTIME_ACCESS |
- READ_ONLY,
- sizeof(efi_vendor_keys),
- &efi_vendor_keys, false);
+ ret = efi_set_variable_int(L"VendorKeys",
+ &efi_global_variable_guid,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_READ_ONLY,
+ sizeof(efi_vendor_keys),
+ &efi_vendor_keys, false);
err:
return ret;
@@ -593,10 +579,9 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
}
#endif /* CONFIG_EFI_SECURE_BOOT */
-static efi_status_t efi_get_variable_common(u16 *variable_name,
- const efi_guid_t *vendor,
- u32 *attributes,
- efi_uintn_t *data_size, void *data)
+efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+ u32 *attributes, efi_uintn_t *data_size,
+ void *data)
{
char *native_name;
efi_status_t ret;
@@ -679,35 +664,6 @@ out:
return ret;
}
-/**
- * efi_efi_get_variable() - retrieve value of a UEFI variable
- *
- * This function implements the GetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @variable_name: name of the variable
- * @vendor: vendor GUID
- * @attributes: attributes of the variable
- * @data_size: size of the buffer to which the variable value is copied
- * @data: buffer to which the variable value is copied
- * Return: status code
- */
-efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
- const efi_guid_t *vendor, u32 *attributes,
- efi_uintn_t *data_size, void *data)
-{
- efi_status_t ret;
-
- EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
- data_size, data);
-
- ret = efi_get_variable_common(variable_name, vendor, attributes,
- data_size, data);
- return EFI_EXIT(ret);
-}
-
static char *efi_variables_list;
static char *efi_cur_variable;
@@ -871,12 +827,9 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
return EFI_EXIT(ret);
}
-static efi_status_t efi_set_variable_common(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(u16 *variable_name, const efi_guid_t *vendor,
+ u32 attributes, efi_uintn_t data_size,
+ const void *data, bool ro_check)
{
char *native_name = NULL, *old_data = NULL, *val = NULL, *s;
efi_uintn_t old_size;
@@ -899,15 +852,15 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
/* check if a variable exists */
old_size = 0;
attr = 0;
- ret = efi_get_variable_common(variable_name, vendor, &attr,
- &old_size, NULL);
+ ret = efi_get_variable_int(variable_name, vendor, &attr,
+ &old_size, NULL);
append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
delete = !append && (!data_size || !attributes);
/* check attributes */
if (old_size) {
- if (ro_check && (attr & READ_ONLY)) {
+ if (ro_check && (attr & EFI_VARIABLE_READ_ONLY)) {
ret = EFI_WRITE_PROTECTED;
goto err;
}
@@ -915,8 +868,8 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
/* attributes won't be changed */
if (!delete &&
((ro_check && attr != attributes) ||
- (!ro_check && ((attr & ~(u32)READ_ONLY)
- != (attributes & ~(u32)READ_ONLY))))) {
+ (!ro_check && ((attr & ~(u32)EFI_VARIABLE_READ_ONLY)
+ != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) {
ret = EFI_INVALID_PARAMETER;
goto err;
}
@@ -990,8 +943,8 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
ret = EFI_OUT_OF_RESOURCES;
goto err;
}
- ret = efi_get_variable_common(variable_name, vendor,
- &attr, &old_size, old_data);
+ ret = efi_get_variable_int(variable_name, vendor,
+ &attr, &old_size, old_data);
if (ret != EFI_SUCCESS)
goto err;
} else {
@@ -1011,7 +964,7 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
/*
* store attributes
*/
- attributes &= (READ_ONLY |
+ attributes &= (EFI_VARIABLE_READ_ONLY |
EFI_VARIABLE_NON_VOLATILE |
EFI_VARIABLE_BOOTSERVICE_ACCESS |
EFI_VARIABLE_RUNTIME_ACCESS |
@@ -1020,7 +973,7 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
while (attributes) {
attr = 1 << (ffs(attributes) - 1);
- if (attr == READ_ONLY) {
+ if (attr == EFI_VARIABLE_READ_ONLY) {
s += sprintf(s, "ro");
} else if (attr == EFI_VARIABLE_NON_VOLATILE) {
s += sprintf(s, "nv");
@@ -1074,12 +1027,12 @@ out:
/* update VendorKeys */
if (vendor_keys_modified & efi_vendor_keys) {
efi_vendor_keys = 0;
- ret = efi_set_variable_common(
+ ret = efi_set_variable_int(
L"VendorKeys",
&efi_global_variable_guid,
EFI_VARIABLE_BOOTSERVICE_ACCESS
| EFI_VARIABLE_RUNTIME_ACCESS
- | READ_ONLY,
+ | EFI_VARIABLE_READ_ONLY,
sizeof(efi_vendor_keys),
&efi_vendor_keys,
false);
@@ -1096,36 +1049,6 @@ err:
return ret;
}
-/**
- * efi_set_variable() - set value of a UEFI variable
- *
- * This function implements the SetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @variable_name: name of the variable
- * @vendor: vendor GUID
- * @attributes: attributes of the variable
- * @data_size: size of the buffer with the variable value
- * @data: buffer with the variable value
- * Return: status code
- */
-efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
- const efi_guid_t *vendor, u32 attributes,
- efi_uintn_t data_size, const void *data)
-{
- EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
- data_size, data);
-
- /* READ_ONLY bit is not part of API */
- attributes &= ~(u32)READ_ONLY;
-
- return EFI_EXIT(efi_set_variable_common(variable_name, vendor,
- attributes, data_size, data,
- true));
-}
-
/**
* efi_query_variable_info() - get information about EFI variables
*
diff --git a/lib/efi_loader/efi_variable_common.c b/lib/efi_loader/efi_variable_common.c
new file mode 100644
index 0000000000..e6a39fceac
--- /dev/null
+++ b/lib/efi_loader/efi_variable_common.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * UEFI runtime variable services
+ *
+ * Copyright (c) 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
+ */
+
+#include <common.h>
+#include <efi_loader.h>
+#include <efi_variable.h>
+#include <linux/bitops.h>
+
+/**
+ * efi_efi_get_variable() - retrieve value of a UEFI variable
+ *
+ * This function implements the GetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @variable_name: name of the variable
+ * @vendor: vendor GUID
+ * @attributes: attributes of the variable
+ * @data_size: size of the buffer to which the variable value is copied
+ * @data: buffer to which the variable value is copied
+ * Return: status code
+ */
+efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
+ const efi_guid_t *vendor, u32 *attributes,
+ efi_uintn_t *data_size, void *data)
+{
+ efi_status_t ret;
+
+ EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
+ data_size, data);
+
+ ret = efi_get_variable_int(variable_name, vendor, attributes,
+ data_size, data);
+
+ /* Remove EFI_VARIABLE_READ_ONLY flag */
+ if (attributes)
+ *attributes &= EFI_VARIABLE_MASK;
+
+ return EFI_EXIT(ret);
+}
+
+/**
+ * efi_set_variable() - set value of a UEFI variable
+ *
+ * This function implements the SetVariable runtime service.
+ *
+ * See the Unified Extensible Firmware Interface (UEFI) specification for
+ * details.
+ *
+ * @variable_name: name of the variable
+ * @vendor: vendor GUID
+ * @attributes: attributes of the variable
+ * @data_size: size of the buffer with the variable value
+ * @data: buffer with the variable value
+ * Return: status code
+ */
+efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
+ const efi_guid_t *vendor, u32 attributes,
+ efi_uintn_t data_size, const void *data)
+{
+ efi_status_t ret;
+
+ EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
+ data_size, data);
+
+ /* Make sure that the EFI_VARIABLE_READ_ONLY flag is not set */
+ if (attributes & ~(u32)EFI_VARIABLE_MASK)
+ ret = EFI_INVALID_PARAMETER;
+ else
+ ret = efi_set_variable_int(variable_name, vendor, attributes,
+ data_size, data, true);
+
+ return EFI_EXIT(ret);
+}
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index cacc76e23d..2513878c82 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -10,6 +10,7 @@
#include <efi.h>
#include <efi_api.h>
#include <efi_loader.h>
+#include <efi_variable.h>
#include <tee.h>
#include <malloc.h>
#include <mm_communication.h>
@@ -243,24 +244,9 @@ out:
return ret;
}
-/**
- * efi_get_variable() - retrieve value of a UEFI variable
- *
- * This function implements the GetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @name: name of the variable
- * @guid: vendor GUID
- * @attr: attributes of the variable
- * @data_size: size of the buffer to which the variable value is copied
- * @data: buffer to which the variable value is copied
- * Return: status code
- */
-efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
- u32 *attr, efi_uintn_t *data_size,
- void *data)
+efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
+ u32 *attributes, efi_uintn_t *data_size,
+ void *data)
{
struct smm_variable_access *var_acc;
efi_uintn_t payload_size;
@@ -269,15 +255,13 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
u8 *comm_buf = NULL;
efi_status_t ret;
- EFI_ENTRY("\"%ls\" %pUl %p %p %p", name, guid, attr, data_size, data);
-
- if (!name || !guid || !data_size) {
+ if (!variable_name || !vendor || !data_size) {
ret = EFI_INVALID_PARAMETER;
goto out;
}
/* Check payload size */
- name_size = u16_strsize(name);
+ name_size = u16_strsize(variable_name);
if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
ret = EFI_INVALID_PARAMETER;
goto out;
@@ -300,11 +284,11 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
goto out;
/* Fill in contents */
- guidcpy(&var_acc->guid, guid);
+ guidcpy(&var_acc->guid, vendor);
var_acc->data_size = tmp_dsize;
var_acc->name_size = name_size;
- var_acc->attr = attr ? *attr : 0;
- memcpy(var_acc->name, name, name_size);
+ var_acc->attr = attributes ? *attributes : 0;
+ memcpy(var_acc->name, variable_name, name_size);
/* Communicate */
ret = mm_communicate(comm_buf, payload_size);
@@ -315,8 +299,8 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
if (ret != EFI_SUCCESS)
goto out;
- if (attr)
- *attr = var_acc->attr;
+ if (attributes)
+ *attributes = var_acc->attr;
if (data)
memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
var_acc->data_size);
@@ -325,7 +309,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
out:
free(comm_buf);
- return EFI_EXIT(ret);
+ return ret;
}
/**
@@ -417,24 +401,9 @@ out:
return EFI_EXIT(ret);
}
-/**
- * efi_set_variable() - set value of a UEFI variable
- *
- * This function implements the SetVariable runtime service.
- *
- * See the Unified Extensible Firmware Interface (UEFI) specification for
- * details.
- *
- * @name: name of the variable
- * @guid: vendor GUID
- * @attr: attributes of the variable
- * @data_size: size of the buffer with the variable value
- * @data: buffer with the variable value
- * Return: status code
- */
-efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
- u32 attr, efi_uintn_t data_size,
- const void *data)
+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)
{
struct smm_variable_access *var_acc;
efi_uintn_t payload_size;
@@ -442,9 +411,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
u8 *comm_buf = NULL;
efi_status_t ret;
- EFI_ENTRY("\"%ls\" %pUl %x %zu %p", name, guid, attr, data_size, data);
-
- if (!name || name[0] == 0 || !guid) {
+ if (!variable_name || variable_name[0] == 0 || !vendor) {
ret = EFI_INVALID_PARAMETER;
goto out;
}
@@ -454,7 +421,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
}
/* Check payload size */
- name_size = u16_strsize(name);
+ name_size = u16_strsize(variable_name);
payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
if (payload_size > max_payload_size) {
ret = EFI_INVALID_PARAMETER;
@@ -468,11 +435,11 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
goto out;
/* Fill in contents */
- guidcpy(&var_acc->guid, guid);
+ guidcpy(&var_acc->guid, vendor);
var_acc->data_size = data_size;
var_acc->name_size = name_size;
- var_acc->attr = attr;
- memcpy(var_acc->name, name, name_size);
+ var_acc->attr = attributes;
+ memcpy(var_acc->name, variable_name, name_size);
memcpy((u8 *)var_acc->name + name_size, data, data_size);
/* Communicate */
@@ -480,7 +447,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
out:
free(comm_buf);
- return EFI_EXIT(ret);
+ return ret;
}
/**
--
2.27.0
^ permalink raw reply related [flat|nested] 4+ messages in thread* [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
2020-06-22 16:10 [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables Heinrich Schuchardt
@ 2020-06-22 23:44 ` AKASHI Takahiro
2020-06-24 5:51 ` Heinrich Schuchardt
0 siblings, 1 reply; 4+ messages in thread
From: AKASHI Takahiro @ 2020-06-22 23:44 UTC (permalink / raw)
To: u-boot
On Mon, Jun 22, 2020 at 06:10:27PM +0200, Heinrich Schuchardt wrote:
> We currently have two implementations of UEFI variables:
>
> * variables provided via an OP-TEE module
> * variables stored in the U-Boot environment
>
> Read only variables are up to now only implemented in the U-Boot
> environment implementation.
>
> Provide a common interface for both implementations that allows handling
> read-only variables.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> v2:
> add missing efi_variable.h
> consider attributes==NULL in efi_variable_get()
> ---
> include/efi_variable.h | 40 +++++++
> lib/efi_loader/Makefile | 1 +
> lib/efi_loader/efi_variable.c | 171 ++++++++-------------------
> lib/efi_loader/efi_variable_common.c | 79 +++++++++++++
> lib/efi_loader/efi_variable_tee.c | 75 ++++--------
> 5 files changed, 188 insertions(+), 178 deletions(-)
> create mode 100644 include/efi_variable.h
> create mode 100644 lib/efi_loader/efi_variable_common.c
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> new file mode 100644
> index 0000000000..784dbd9cf4
> --- /dev/null
> +++ b/include/efi_variable.h
I think that all the stuff here should be put in efi_loader.h.
I don't see any benefit of having a separate header.
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (c) 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + */
> +
> +#ifndef _EFI_VARIABLE_H
> +#define _EFI_VARIABLE_H
> +
> +#define EFI_VARIABLE_READ_ONLY BIT(31)
This is not part of UEFI specification.
Having the same prefix, EFI_VARIABLE_, as other attributes
can be confusing.
-Takahiro Akashi
> +/**
> + * efi_get_variable() - retrieve value of a UEFI variable
> + *
> + * @variable_name: name of the variable
> + * @vendor: vendor GUID
> + * @attributes: attributes of the variable
> + * @data_size: size of the buffer to which the variable value is copied
> + * @data: buffer to which the variable value is copied
> + * Return: status code
> + */
> +efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> + u32 *attributes, efi_uintn_t *data_size,
> + void *data);
> +
> +/**
> + * efi_set_variable() - set value of a UEFI variable
> + *
> + * @variable_name: name of the variable
> + * @vendor: vendor GUID
> + * @attributes: attributes of the variable
> + * @data_size: size of the buffer with the variable value
> + * @data: buffer with the variable value
> + * @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,
> + u32 attributes, efi_uintn_t data_size,
> + const void *data, bool ro_check);
> +
> +#endif
> diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> index 57c7e66ea0..16b93ef7f0 100644
> --- a/lib/efi_loader/Makefile
> +++ b/lib/efi_loader/Makefile
> @@ -35,6 +35,7 @@ obj-y += efi_root_node.o
> obj-y += efi_runtime.o
> obj-y += efi_setup.o
> obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> +obj-y += efi_variable_common.o
> ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> obj-y += efi_variable_tee.o
> else
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index e097670e28..85df1427bc 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -7,6 +7,7 @@
>
> #include <common.h>
> #include <efi_loader.h>
> +#include <efi_variable.h>
> #include <env.h>
> #include <env_internal.h>
> #include <hexdump.h>
> @@ -15,7 +16,6 @@
> #include <search.h>
> #include <uuid.h>
> #include <crypto/pkcs7_parser.h>
> -#include <linux/bitops.h>
> #include <linux/compat.h>
> #include <u-boot/crc.h>
>
> @@ -30,20 +30,6 @@ static bool efi_secure_boot;
> static int efi_secure_mode;
> static u8 efi_vendor_keys;
>
> -#define READ_ONLY BIT(31)
> -
> -static efi_status_t efi_get_variable_common(u16 *variable_name,
> - const efi_guid_t *vendor,
> - u32 *attributes,
> - efi_uintn_t *data_size, void *data);
> -
> -static efi_status_t efi_set_variable_common(u16 *variable_name,
> - const efi_guid_t *vendor,
> - u32 attributes,
> - efi_uintn_t data_size,
> - const void *data,
> - bool ro_check);
> -
> /*
> * Mapping between EFI variables and u-boot variables:
> *
> @@ -154,7 +140,7 @@ static const char *parse_attr(const char *str, u32 *attrp, u64 *timep)
> str++;
>
> if ((s = prefix(str, "ro"))) {
> - attr |= READ_ONLY;
> + attr |= EFI_VARIABLE_READ_ONLY;
> } else if ((s = prefix(str, "nv"))) {
> attr |= EFI_VARIABLE_NON_VOLATILE;
> } else if ((s = prefix(str, "boot"))) {
> @@ -202,29 +188,29 @@ static efi_status_t efi_set_secure_state(int sec_boot, int setup_mode,
>
> attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> EFI_VARIABLE_RUNTIME_ACCESS |
> - READ_ONLY;
> - ret = efi_set_variable_common(L"SecureBoot", &efi_global_variable_guid,
> - attributes, sizeof(sec_boot), &sec_boot,
> - false);
> + EFI_VARIABLE_READ_ONLY;
> + ret = efi_set_variable_int(L"SecureBoot", &efi_global_variable_guid,
> + attributes, sizeof(sec_boot), &sec_boot,
> + false);
> if (ret != EFI_SUCCESS)
> goto err;
>
> - ret = efi_set_variable_common(L"SetupMode", &efi_global_variable_guid,
> - attributes, sizeof(setup_mode),
> - &setup_mode, false);
> + ret = efi_set_variable_int(L"SetupMode", &efi_global_variable_guid,
> + attributes, sizeof(setup_mode),
> + &setup_mode, false);
> if (ret != EFI_SUCCESS)
> goto err;
>
> - ret = efi_set_variable_common(L"AuditMode", &efi_global_variable_guid,
> - attributes, sizeof(audit_mode),
> - &audit_mode, false);
> + ret = efi_set_variable_int(L"AuditMode", &efi_global_variable_guid,
> + attributes, sizeof(audit_mode),
> + &audit_mode, false);
> if (ret != EFI_SUCCESS)
> goto err;
>
> - ret = efi_set_variable_common(L"DeployedMode",
> - &efi_global_variable_guid, attributes,
> - sizeof(deployed_mode), &deployed_mode,
> - false);
> + ret = efi_set_variable_int(L"DeployedMode",
> + &efi_global_variable_guid, attributes,
> + sizeof(deployed_mode), &deployed_mode,
> + false);
> err:
> return ret;
> }
> @@ -234,7 +220,7 @@ err:
> * @mode: new state
> *
> * Depending on @mode, secure boot related variables are updated.
> - * Those variables are *read-only* for users, efi_set_variable_common()
> + * Those variables are *read-only* for users, efi_set_variable_int()
> * is called here.
> *
> * Return: status code
> @@ -252,10 +238,10 @@ static efi_status_t efi_transfer_secure_state(enum efi_secure_mode mode)
>
> efi_secure_boot = true;
> } else if (mode == EFI_MODE_AUDIT) {
> - ret = efi_set_variable_common(L"PK", &efi_global_variable_guid,
> - EFI_VARIABLE_BOOTSERVICE_ACCESS |
> - EFI_VARIABLE_RUNTIME_ACCESS,
> - 0, NULL, false);
> + 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_SUCCESS)
> goto err;
>
> @@ -307,8 +293,8 @@ static efi_status_t efi_init_secure_state(void)
> */
>
> size = 0;
> - ret = efi_get_variable_common(L"PK", &efi_global_variable_guid,
> - NULL, &size, NULL);
> + ret = efi_get_variable_int(L"PK", &efi_global_variable_guid,
> + NULL, &size, NULL);
> if (ret == EFI_BUFFER_TOO_SMALL) {
> if (IS_ENABLED(CONFIG_EFI_SECURE_BOOT))
> mode = EFI_MODE_USER;
> @@ -325,13 +311,13 @@ static efi_status_t efi_init_secure_state(void)
>
> ret = efi_transfer_secure_state(mode);
> if (ret == EFI_SUCCESS)
> - ret = efi_set_variable_common(L"VendorKeys",
> - &efi_global_variable_guid,
> - EFI_VARIABLE_BOOTSERVICE_ACCESS |
> - EFI_VARIABLE_RUNTIME_ACCESS |
> - READ_ONLY,
> - sizeof(efi_vendor_keys),
> - &efi_vendor_keys, false);
> + ret = efi_set_variable_int(L"VendorKeys",
> + &efi_global_variable_guid,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_READ_ONLY,
> + sizeof(efi_vendor_keys),
> + &efi_vendor_keys, false);
>
> err:
> return ret;
> @@ -593,10 +579,9 @@ static efi_status_t efi_variable_authenticate(u16 *variable,
> }
> #endif /* CONFIG_EFI_SECURE_BOOT */
>
> -static efi_status_t efi_get_variable_common(u16 *variable_name,
> - const efi_guid_t *vendor,
> - u32 *attributes,
> - efi_uintn_t *data_size, void *data)
> +efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> + u32 *attributes, efi_uintn_t *data_size,
> + void *data)
> {
> char *native_name;
> efi_status_t ret;
> @@ -679,35 +664,6 @@ out:
> return ret;
> }
>
> -/**
> - * efi_efi_get_variable() - retrieve value of a UEFI variable
> - *
> - * This function implements the GetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @variable_name: name of the variable
> - * @vendor: vendor GUID
> - * @attributes: attributes of the variable
> - * @data_size: size of the buffer to which the variable value is copied
> - * @data: buffer to which the variable value is copied
> - * Return: status code
> - */
> -efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> - const efi_guid_t *vendor, u32 *attributes,
> - efi_uintn_t *data_size, void *data)
> -{
> - efi_status_t ret;
> -
> - EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> - data_size, data);
> -
> - ret = efi_get_variable_common(variable_name, vendor, attributes,
> - data_size, data);
> - return EFI_EXIT(ret);
> -}
> -
> static char *efi_variables_list;
> static char *efi_cur_variable;
>
> @@ -871,12 +827,9 @@ efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> return EFI_EXIT(ret);
> }
>
> -static efi_status_t efi_set_variable_common(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(u16 *variable_name, const efi_guid_t *vendor,
> + u32 attributes, efi_uintn_t data_size,
> + const void *data, bool ro_check)
> {
> char *native_name = NULL, *old_data = NULL, *val = NULL, *s;
> efi_uintn_t old_size;
> @@ -899,15 +852,15 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
> /* check if a variable exists */
> old_size = 0;
> attr = 0;
> - ret = efi_get_variable_common(variable_name, vendor, &attr,
> - &old_size, NULL);
> + ret = efi_get_variable_int(variable_name, vendor, &attr,
> + &old_size, NULL);
> append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> attributes &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
> delete = !append && (!data_size || !attributes);
>
> /* check attributes */
> if (old_size) {
> - if (ro_check && (attr & READ_ONLY)) {
> + if (ro_check && (attr & EFI_VARIABLE_READ_ONLY)) {
> ret = EFI_WRITE_PROTECTED;
> goto err;
> }
> @@ -915,8 +868,8 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
> /* attributes won't be changed */
> if (!delete &&
> ((ro_check && attr != attributes) ||
> - (!ro_check && ((attr & ~(u32)READ_ONLY)
> - != (attributes & ~(u32)READ_ONLY))))) {
> + (!ro_check && ((attr & ~(u32)EFI_VARIABLE_READ_ONLY)
> + != (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))) {
> ret = EFI_INVALID_PARAMETER;
> goto err;
> }
> @@ -990,8 +943,8 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
> ret = EFI_OUT_OF_RESOURCES;
> goto err;
> }
> - ret = efi_get_variable_common(variable_name, vendor,
> - &attr, &old_size, old_data);
> + ret = efi_get_variable_int(variable_name, vendor,
> + &attr, &old_size, old_data);
> if (ret != EFI_SUCCESS)
> goto err;
> } else {
> @@ -1011,7 +964,7 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
> /*
> * store attributes
> */
> - attributes &= (READ_ONLY |
> + attributes &= (EFI_VARIABLE_READ_ONLY |
> EFI_VARIABLE_NON_VOLATILE |
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> EFI_VARIABLE_RUNTIME_ACCESS |
> @@ -1020,7 +973,7 @@ static efi_status_t efi_set_variable_common(u16 *variable_name,
> while (attributes) {
> attr = 1 << (ffs(attributes) - 1);
>
> - if (attr == READ_ONLY) {
> + if (attr == EFI_VARIABLE_READ_ONLY) {
> s += sprintf(s, "ro");
> } else if (attr == EFI_VARIABLE_NON_VOLATILE) {
> s += sprintf(s, "nv");
> @@ -1074,12 +1027,12 @@ out:
> /* update VendorKeys */
> if (vendor_keys_modified & efi_vendor_keys) {
> efi_vendor_keys = 0;
> - ret = efi_set_variable_common(
> + ret = efi_set_variable_int(
> L"VendorKeys",
> &efi_global_variable_guid,
> EFI_VARIABLE_BOOTSERVICE_ACCESS
> | EFI_VARIABLE_RUNTIME_ACCESS
> - | READ_ONLY,
> + | EFI_VARIABLE_READ_ONLY,
> sizeof(efi_vendor_keys),
> &efi_vendor_keys,
> false);
> @@ -1096,36 +1049,6 @@ err:
> return ret;
> }
>
> -/**
> - * efi_set_variable() - set value of a UEFI variable
> - *
> - * This function implements the SetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @variable_name: name of the variable
> - * @vendor: vendor GUID
> - * @attributes: attributes of the variable
> - * @data_size: size of the buffer with the variable value
> - * @data: buffer with the variable value
> - * Return: status code
> - */
> -efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> - const efi_guid_t *vendor, u32 attributes,
> - efi_uintn_t data_size, const void *data)
> -{
> - EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> - data_size, data);
> -
> - /* READ_ONLY bit is not part of API */
> - attributes &= ~(u32)READ_ONLY;
> -
> - return EFI_EXIT(efi_set_variable_common(variable_name, vendor,
> - attributes, data_size, data,
> - true));
> -}
> -
> /**
> * efi_query_variable_info() - get information about EFI variables
> *
> diff --git a/lib/efi_loader/efi_variable_common.c b/lib/efi_loader/efi_variable_common.c
> new file mode 100644
> index 0000000000..e6a39fceac
> --- /dev/null
> +++ b/lib/efi_loader/efi_variable_common.c
> @@ -0,0 +1,79 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * UEFI runtime variable services
> + *
> + * Copyright (c) 2020, Heinrich Schuchardt <xypron.glpk@gmx.de>
> + */
> +
> +#include <common.h>
> +#include <efi_loader.h>
> +#include <efi_variable.h>
> +#include <linux/bitops.h>
> +
> +/**
> + * efi_efi_get_variable() - retrieve value of a UEFI variable
> + *
> + * This function implements the GetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @variable_name: name of the variable
> + * @vendor: vendor GUID
> + * @attributes: attributes of the variable
> + * @data_size: size of the buffer to which the variable value is copied
> + * @data: buffer to which the variable value is copied
> + * Return: status code
> + */
> +efi_status_t EFIAPI efi_get_variable(u16 *variable_name,
> + const efi_guid_t *vendor, u32 *attributes,
> + efi_uintn_t *data_size, void *data)
> +{
> + efi_status_t ret;
> +
> + EFI_ENTRY("\"%ls\" %pUl %p %p %p", variable_name, vendor, attributes,
> + data_size, data);
> +
> + ret = efi_get_variable_int(variable_name, vendor, attributes,
> + data_size, data);
> +
> + /* Remove EFI_VARIABLE_READ_ONLY flag */
> + if (attributes)
> + *attributes &= EFI_VARIABLE_MASK;
> +
> + return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_set_variable() - set value of a UEFI variable
> + *
> + * This function implements the SetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @variable_name: name of the variable
> + * @vendor: vendor GUID
> + * @attributes: attributes of the variable
> + * @data_size: size of the buffer with the variable value
> + * @data: buffer with the variable value
> + * Return: status code
> + */
> +efi_status_t EFIAPI efi_set_variable(u16 *variable_name,
> + const efi_guid_t *vendor, u32 attributes,
> + efi_uintn_t data_size, const void *data)
> +{
> + efi_status_t ret;
> +
> + EFI_ENTRY("\"%ls\" %pUl %x %zu %p", variable_name, vendor, attributes,
> + data_size, data);
> +
> + /* Make sure that the EFI_VARIABLE_READ_ONLY flag is not set */
> + if (attributes & ~(u32)EFI_VARIABLE_MASK)
> + ret = EFI_INVALID_PARAMETER;
> + else
> + ret = efi_set_variable_int(variable_name, vendor, attributes,
> + data_size, data, true);
> +
> + return EFI_EXIT(ret);
> +}
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index cacc76e23d..2513878c82 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -10,6 +10,7 @@
> #include <efi.h>
> #include <efi_api.h>
> #include <efi_loader.h>
> +#include <efi_variable.h>
> #include <tee.h>
> #include <malloc.h>
> #include <mm_communication.h>
> @@ -243,24 +244,9 @@ out:
> return ret;
> }
>
> -/**
> - * efi_get_variable() - retrieve value of a UEFI variable
> - *
> - * This function implements the GetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @name: name of the variable
> - * @guid: vendor GUID
> - * @attr: attributes of the variable
> - * @data_size: size of the buffer to which the variable value is copied
> - * @data: buffer to which the variable value is copied
> - * Return: status code
> - */
> -efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> - u32 *attr, efi_uintn_t *data_size,
> - void *data)
> +efi_status_t efi_get_variable_int(u16 *variable_name, const efi_guid_t *vendor,
> + u32 *attributes, efi_uintn_t *data_size,
> + void *data)
> {
> struct smm_variable_access *var_acc;
> efi_uintn_t payload_size;
> @@ -269,15 +255,13 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> u8 *comm_buf = NULL;
> efi_status_t ret;
>
> - EFI_ENTRY("\"%ls\" %pUl %p %p %p", name, guid, attr, data_size, data);
> -
> - if (!name || !guid || !data_size) {
> + if (!variable_name || !vendor || !data_size) {
> ret = EFI_INVALID_PARAMETER;
> goto out;
> }
>
> /* Check payload size */
> - name_size = u16_strsize(name);
> + name_size = u16_strsize(variable_name);
> if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> ret = EFI_INVALID_PARAMETER;
> goto out;
> @@ -300,11 +284,11 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> goto out;
>
> /* Fill in contents */
> - guidcpy(&var_acc->guid, guid);
> + guidcpy(&var_acc->guid, vendor);
> var_acc->data_size = tmp_dsize;
> var_acc->name_size = name_size;
> - var_acc->attr = attr ? *attr : 0;
> - memcpy(var_acc->name, name, name_size);
> + var_acc->attr = attributes ? *attributes : 0;
> + memcpy(var_acc->name, variable_name, name_size);
>
> /* Communicate */
> ret = mm_communicate(comm_buf, payload_size);
> @@ -315,8 +299,8 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> if (ret != EFI_SUCCESS)
> goto out;
>
> - if (attr)
> - *attr = var_acc->attr;
> + if (attributes)
> + *attributes = var_acc->attr;
> if (data)
> memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> var_acc->data_size);
> @@ -325,7 +309,7 @@ efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
>
> out:
> free(comm_buf);
> - return EFI_EXIT(ret);
> + return ret;
> }
>
> /**
> @@ -417,24 +401,9 @@ out:
> return EFI_EXIT(ret);
> }
>
> -/**
> - * efi_set_variable() - set value of a UEFI variable
> - *
> - * This function implements the SetVariable runtime service.
> - *
> - * See the Unified Extensible Firmware Interface (UEFI) specification for
> - * details.
> - *
> - * @name: name of the variable
> - * @guid: vendor GUID
> - * @attr: attributes of the variable
> - * @data_size: size of the buffer with the variable value
> - * @data: buffer with the variable value
> - * Return: status code
> - */
> -efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> - u32 attr, efi_uintn_t data_size,
> - const void *data)
> +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)
> {
> struct smm_variable_access *var_acc;
> efi_uintn_t payload_size;
> @@ -442,9 +411,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> u8 *comm_buf = NULL;
> efi_status_t ret;
>
> - EFI_ENTRY("\"%ls\" %pUl %x %zu %p", name, guid, attr, data_size, data);
> -
> - if (!name || name[0] == 0 || !guid) {
> + if (!variable_name || variable_name[0] == 0 || !vendor) {
> ret = EFI_INVALID_PARAMETER;
> goto out;
> }
> @@ -454,7 +421,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> }
>
> /* Check payload size */
> - name_size = u16_strsize(name);
> + name_size = u16_strsize(variable_name);
> payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> if (payload_size > max_payload_size) {
> ret = EFI_INVALID_PARAMETER;
> @@ -468,11 +435,11 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> goto out;
>
> /* Fill in contents */
> - guidcpy(&var_acc->guid, guid);
> + guidcpy(&var_acc->guid, vendor);
> var_acc->data_size = data_size;
> var_acc->name_size = name_size;
> - var_acc->attr = attr;
> - memcpy(var_acc->name, name, name_size);
> + var_acc->attr = attributes;
> + memcpy(var_acc->name, variable_name, name_size);
> memcpy((u8 *)var_acc->name + name_size, data, data_size);
>
> /* Communicate */
> @@ -480,7 +447,7 @@ efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
>
> out:
> free(comm_buf);
> - return EFI_EXIT(ret);
> + return ret;
> }
>
> /**
> --
> 2.27.0
>
^ permalink raw reply [flat|nested] 4+ messages in thread* [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
2020-06-22 23:44 ` AKASHI Takahiro
@ 2020-06-24 5:51 ` Heinrich Schuchardt
2020-06-24 6:29 ` AKASHI Takahiro
0 siblings, 1 reply; 4+ messages in thread
From: Heinrich Schuchardt @ 2020-06-24 5:51 UTC (permalink / raw)
To: u-boot
On 6/23/20 1:44 AM, AKASHI Takahiro wrote:
> On Mon, Jun 22, 2020 at 06:10:27PM +0200, Heinrich Schuchardt wrote:
>> We currently have two implementations of UEFI variables:
>>
>> * variables provided via an OP-TEE module
>> * variables stored in the U-Boot environment
>>
>> Read only variables are up to now only implemented in the U-Boot
>> environment implementation.
>>
>> Provide a common interface for both implementations that allows handling
>> read-only variables.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> v2:
>> add missing efi_variable.h
>> consider attributes==NULL in efi_variable_get()
>> ---
>> include/efi_variable.h | 40 +++++++
>> lib/efi_loader/Makefile | 1 +
>> lib/efi_loader/efi_variable.c | 171 ++++++++-------------------
>> lib/efi_loader/efi_variable_common.c | 79 +++++++++++++
>> lib/efi_loader/efi_variable_tee.c | 75 ++++--------
>> 5 files changed, 188 insertions(+), 178 deletions(-)
>> create mode 100644 include/efi_variable.h
>> create mode 100644 lib/efi_loader/efi_variable_common.c
>>
>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>> new file mode 100644
>> index 0000000000..784dbd9cf4
>> --- /dev/null
>> +++ b/include/efi_variable.h
>
> I think that all the stuff here should be put in efi_loader.h.
> I don't see any benefit of having a separate header.
>
>
This is more or less a question of taste. My motivation is:
* efi_loader.h is rather large (805 lines).
* Other variable functions will be added.
* The functions defined here are used only in very few places
while efi_loader.h is included in 57 files.
Best regards
Heinrich
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
2020-06-24 5:51 ` Heinrich Schuchardt
@ 2020-06-24 6:29 ` AKASHI Takahiro
0 siblings, 0 replies; 4+ messages in thread
From: AKASHI Takahiro @ 2020-06-24 6:29 UTC (permalink / raw)
To: u-boot
On Wed, Jun 24, 2020 at 07:51:42AM +0200, Heinrich Schuchardt wrote:
> On 6/23/20 1:44 AM, AKASHI Takahiro wrote:
> > On Mon, Jun 22, 2020 at 06:10:27PM +0200, Heinrich Schuchardt wrote:
> >> We currently have two implementations of UEFI variables:
> >>
> >> * variables provided via an OP-TEE module
> >> * variables stored in the U-Boot environment
> >>
> >> Read only variables are up to now only implemented in the U-Boot
> >> environment implementation.
> >>
> >> Provide a common interface for both implementations that allows handling
> >> read-only variables.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> v2:
> >> add missing efi_variable.h
> >> consider attributes==NULL in efi_variable_get()
> >> ---
> >> include/efi_variable.h | 40 +++++++
> >> lib/efi_loader/Makefile | 1 +
> >> lib/efi_loader/efi_variable.c | 171 ++++++++-------------------
> >> lib/efi_loader/efi_variable_common.c | 79 +++++++++++++
> >> lib/efi_loader/efi_variable_tee.c | 75 ++++--------
> >> 5 files changed, 188 insertions(+), 178 deletions(-)
> >> create mode 100644 include/efi_variable.h
> >> create mode 100644 lib/efi_loader/efi_variable_common.c
> >>
> >> diff --git a/include/efi_variable.h b/include/efi_variable.h
> >> new file mode 100644
> >> index 0000000000..784dbd9cf4
> >> --- /dev/null
> >> +++ b/include/efi_variable.h
> >
> > I think that all the stuff here should be put in efi_loader.h.
> > I don't see any benefit of having a separate header.
> >
> >
>
> This is more or less a question of taste. My motivation is:
I can agree, but at the same time, I don't like such an ad-hoc
confusing approach. I think that you should have a firm discipline.
> * efi_loader.h is rather large (805 lines).
> * Other variable functions will be added.
> * The functions defined here are used only in very few places
> while efi_loader.h is included in 57 files.
If we allow this, we will have a number of small headers,
which will contradict a notion of efi_loader.h.
-Takahiro Akashi
> Best regards
>
> Heinrich
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-06-24 6:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-22 16:10 [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables Heinrich Schuchardt
2020-06-22 23:44 ` AKASHI Takahiro
2020-06-24 5:51 ` Heinrich Schuchardt
2020-06-24 6:29 ` AKASHI Takahiro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox