Re: [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
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 > >> --- > >> 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 00..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
Re: [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
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 >> --- >> 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 00..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
Re: [PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
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
> ---
> 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 00..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
> + */
> +
> +#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
> #include
> +#include
> #include
> #include
> #include
> @@ -15,7 +16,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
>
> @@ -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 |=
[PATCH v2 1/1] efi_loader: prepare for read only OP-TEE variables
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
---
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 00..784dbd9cf4
--- /dev/null
+++ b/include/efi_variable.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (c) 2020, Heinrich Schuchardt
+ */
+
+#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
#include
+#include
#include
#include
#include
@@ -15,7 +16,6 @@
#include
#include
#include
-#include
#include
#include
@@ -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", _global_variable_guid,
- attributes, sizeof(sec_boot),
