* [PATCH v3 0/4]
@ 2024-04-18 12:54 Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 1/4] efi_loader: conditionally enable SetvariableRT Ilias Apalodimas
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 12:54 UTC (permalink / raw)
To: xypron.glpk, kettenis
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Ilias Apalodimas, Tom Rini, Masahisa Kojima,
AKASHI Takahiro, Raymond Mao, Matthias Schiffer, Simon Glass,
Janne Grunau, Abdellatif El Khlifi, Sughosh Ganu,
Richard Henderson, Sam Edwards, Alper Nebi Yasak, Weizhao Ouyang,
u-boot
Hi!
This is v3 of SetVariable at runtime [0]
Nothing changed drastically from v2.
A few more test cases have been added, comments/suggestions have been
addressed and a bug where deleting a variable by setting 'attributes' to
0 has been fixed.
Changes since v2:
- Add more selftests checking for variable deletion as well as the
VarToFile header format
- Use unaligned sized variables on tests
- Add a new function to get the variable entry length instead of
repurposing efi_var_mem_compare()
- Converted VarToFile to RO
- Added a few comments requested by Heinrich
- Fixed a bug where SetVariable with attributes set to 0 did not delete
the variable but returned EFI_INVALID_PARAMETER instead
- Run FWTS 'uefitests' and attach the log in patch #1
- Added r-b tags from Heinrich
Changes since v1:
- Instead of Creating VarToFile at SetVariable, create it on GetVariable.
This allows us to get rid of the preallocated RT buffer, since the
address is user provided
- convert Set/GetVariableRT -> Set/GetVariable at runtime
- return EFI_INVALID_PARAM is NV is not set at runtime
- Heinrich sent me the efi_var_collect_mem() variant
Changes since the RFC:
- Return EFI_INVALID_PARAM if attributes are not volatile
- Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
- Add 2 EFI variables and allow userspace to write the file
- Add selftests
I also have a patch enable QueryVariableInfo [1], but I don't want to
introduce new patches now. This also needs a few more testcases of its
own so I'll send it once we finalize this one.
[0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
[1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
Regards
/Ilias
Ilias Apalodimas (4):
efi_loader: conditionally enable SetvariableRT
efi_loader: Add OS notifications for SetVariable at runtime
efi_loader: add an EFI variable with the file contents
efi_selftest: add tests for setvariableRT
include/efi_loader.h | 4 +
include/efi_variable.h | 16 +-
lib/charset.c | 2 +-
lib/efi_loader/Kconfig | 16 ++
lib/efi_loader/efi_runtime.c | 42 ++++
lib/efi_loader/efi_var_common.c | 6 +-
lib/efi_loader/efi_var_mem.c | 151 ++++++++-----
lib/efi_loader/efi_variable.c | 122 ++++++++--
lib/efi_loader/efi_variable_tee.c | 5 -
.../efi_selftest_variables_runtime.c | 211 +++++++++++++++++-
10 files changed, 495 insertions(+), 80 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v3 1/4] efi_loader: conditionally enable SetvariableRT
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
@ 2024-04-18 12:54 ` Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 2/4] efi_loader: Add OS notifications for SetVariable at runtime Ilias Apalodimas
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 12:54 UTC (permalink / raw)
To: xypron.glpk, kettenis
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Ilias Apalodimas, Heinrich Schuchardt, Tom Rini,
Masahisa Kojima, AKASHI Takahiro, Raymond Mao, Janne Grunau,
Simon Glass, Matthias Schiffer, Abdellatif El Khlifi,
Sughosh Ganu, Richard Henderson, Sam Edwards, Alper Nebi Yasak,
Weizhao Ouyang, u-boot
When we store EFI variables on file we don't allow SetVariable at runtime,
since the OS doesn't know how to access or write that file. At the same
time keeping the U-Boot drivers alive in runtime sections and performing
writes from the firmware is dangerous -- if at all possible.
For GetVariable at runtime we copy runtime variables in RAM and expose them
to the OS. Add a Kconfig option and provide SetVariable at runtime using
the same memory backend. The OS will be responsible for syncing the RAM
contents to the file, otherwise any changes made during runtime won't
persist reboots.
It's worth noting that the variable store format is defined in EBBR [0]
and authenticated variables are explicitly prohibited, since they have
to be stored on a medium that's tamper and rollback protected.
- pre-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
$~ efibootmgr -n 0001
Could not set BootNext: Read-only file system
- post-patch
$~ mount | grep efiva
efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
$~ efibootmgr -n 0001
BootNext: 0001
BootCurrent: 0000
BootOrder: 0000,0001
Boot0000* debian HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
Boot0001* virtio 0 VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
$~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
Name: "BootNext"
Attributes:
Non-Volatile
Boot Service Access
Runtime Service Access
Value:
00000000 01 00
FWTS runtime results
Skipped tests are for SetVariable which is now supported
'Passed' test is for QueryVariableInfo which is not yet supported
Test: UEFI miscellaneous runtime service interface tests.
Test for UEFI miscellaneous runtime service interfaces 6 skipped
Stress test for UEFI miscellaneous runtime service i.. 1 skipped
Test GetNextHighMonotonicCount with invalid NULL par.. 1 skipped
Test UEFI miscellaneous runtime services unsupported.. 1 passed
Test: UEFI Runtime service variable interface tests.
Test UEFI RT service get variable interface. 1 passed
Test UEFI RT service get next variable name interface. 4 passed
Test UEFI RT service set variable interface. 8 passed
Test UEFI RT service query variable info interface. 1 skipped
Test UEFI RT service variable interface stress test. 2 passed
Test UEFI RT service set variable interface stress t.. 4 passed
Test UEFI RT service query variable info interface s.. 1 skipped
Test UEFI RT service get variable interface, invalid.. 5 passed
Test UEFI RT variable services unsupported status. 1 passed, 3 skipped
[0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
lib/efi_loader/Kconfig | 16 +++
lib/efi_loader/efi_runtime.c | 4 +
lib/efi_loader/efi_variable.c | 116 ++++++++++++++++--
.../efi_selftest_variables_runtime.c | 14 ++-
4 files changed, 136 insertions(+), 14 deletions(-)
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index e13a6f9f4c3a..cc8371a3bb4c 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -62,6 +62,22 @@ config EFI_VARIABLE_FILE_STORE
Select this option if you want non-volatile UEFI variables to be
stored as file /ubootefi.var on the EFI system partition.
+config EFI_RT_VOLATILE_STORE
+ bool "Allow variable runtime services in volatile storage (e.g RAM)"
+ depends on EFI_VARIABLE_FILE_STORE
+ help
+ When EFI variables are stored on file we don't allow SetVariableRT,
+ since the OS doesn't know how to write that file. At he same time
+ we copy runtime variables in DRAM and support GetVariableRT
+
+ Enable this option to allow SetVariableRT on the RAM backend of
+ the EFI variable storage. The OS will be responsible for syncing
+ the RAM contents to the file, otherwise any changes made during
+ runtime won't persist reboots.
+ Authenticated variables are not supported. Note that this will
+ violate the EFI spec since writing auth variables will return
+ EFI_INVALID_PARAMETER
+
config EFI_MM_COMM_TEE
bool "UEFI variables storage service via the trusted world"
depends on OPTEE
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index a61c9a77b13f..dde083b09665 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -127,6 +127,10 @@ efi_status_t efi_init_runtime_supported(void)
EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
EFI_RT_SUPPORTED_CONVERT_POINTER;
+ if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
+ rt_table->runtime_services_supported |=
+ EFI_RT_SUPPORTED_SET_VARIABLE;
+
/*
* This value must be synced with efi_runtime_detach_list
* as well as efi_runtime_services.
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index e6c1219a11c8..47bb79920575 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -219,17 +219,20 @@ 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(const u16 *variable_name,
- const efi_guid_t *vendor,
- u32 attributes, efi_uintn_t data_size,
- const void *data, bool ro_check)
+/**
+ * setvariable_allowed() - checks defined by the UEFI spec for setvariable
+ *
+ * @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
+ */
+static efi_status_t __efi_runtime
+setvariable_allowed(const u16 *variable_name, const efi_guid_t *vendor,
+ u32 attributes, efi_uintn_t data_size, const void *data)
{
- struct efi_var_entry *var;
- efi_uintn_t ret;
- bool append, delete;
- u64 time = 0;
- enum efi_auth_var_type var_type;
-
if (!variable_name || !*variable_name || !vendor)
return EFI_INVALID_PARAMETER;
@@ -261,6 +264,25 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
!(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)))
return EFI_INVALID_PARAMETER;
+ return EFI_SUCCESS;
+}
+
+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)
+{
+ struct efi_var_entry *var;
+ efi_uintn_t ret;
+ bool append, delete;
+ u64 time = 0;
+ enum efi_auth_var_type var_type;
+
+ ret = setvariable_allowed(variable_name, vendor, attributes, data_size,
+ data);
+ if (ret != EFI_SUCCESS)
+ return ret;
+
/* check if a variable exists */
var = efi_var_mem_find(vendor, variable_name, NULL);
append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
@@ -454,7 +476,79 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
u32 attributes, efi_uintn_t data_size,
const void *data)
{
- return EFI_UNSUPPORTED;
+ struct efi_var_entry *var;
+ efi_uintn_t ret;
+ bool append, delete;
+ u64 time = 0;
+
+ if (!IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
+ return EFI_UNSUPPORTED;
+
+ /*
+ * Authenticated variables are not supported. The EFI spec
+ * in §32.3.6 requires keys to be stored in non-volatile storage which
+ * is tamper and delete resistant.
+ * The rest of the checks are in setvariable_allowed()
+ */
+ if (attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)
+ return EFI_INVALID_PARAMETER;
+
+ ret = setvariable_allowed(variable_name, vendor, attributes, data_size,
+ data);
+ if (ret != EFI_SUCCESS)
+ return ret;
+
+ /* check if a variable exists */
+ var = efi_var_mem_find(vendor, variable_name, NULL);
+ append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
+ attributes &= ~EFI_VARIABLE_APPEND_WRITE;
+ delete = !append && (!data_size || !attributes);
+
+ /* BS only variables are hidden deny writing them */
+ if (!delete && !(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
+ return EFI_INVALID_PARAMETER;
+
+ if (var) {
+ if (var->attr & EFI_VARIABLE_READ_ONLY ||
+ !(var->attr & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_WRITE_PROTECTED;
+
+ /* attributes won't be changed */
+ if (!delete && (((var->attr & ~EFI_VARIABLE_READ_ONLY) !=
+ (attributes & ~EFI_VARIABLE_READ_ONLY))))
+ return EFI_INVALID_PARAMETER;
+ time = var->time;
+ } else {
+ if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
+ return EFI_INVALID_PARAMETER;
+ if (append && !data_size)
+ return EFI_SUCCESS;
+ if (delete)
+ return EFI_NOT_FOUND;
+ }
+
+ if (delete) {
+ /* EFI_NOT_FOUND has been handled before */
+ attributes = var->attr;
+ ret = EFI_SUCCESS;
+ } else if (append && var) {
+ u16 *old_data = (void *)((uintptr_t)var->name +
+ sizeof(u16) * (u16_strlen(var->name) + 1));
+
+ ret = efi_var_mem_ins(variable_name, vendor, attributes,
+ var->length, old_data, data_size, data,
+ time);
+ } else {
+ ret = efi_var_mem_ins(variable_name, vendor, attributes,
+ data_size, data, 0, NULL, time);
+ }
+
+ if (ret != EFI_SUCCESS)
+ return ret;
+ /* We are always inserting new variables, get rid of the old copy */
+ efi_var_mem_del(var);
+
+ return EFI_SUCCESS;
}
/**
diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
index 4700d9424105..986924b881dd 100644
--- a/lib/efi_selftest/efi_selftest_variables_runtime.c
+++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
@@ -62,9 +62,17 @@ static int execute(void)
EFI_VARIABLE_BOOTSERVICE_ACCESS |
EFI_VARIABLE_RUNTIME_ACCESS,
3, v + 4);
- if (ret != EFI_UNSUPPORTED) {
- efi_st_error("SetVariable failed\n");
- return EFI_ST_FAILURE;
+ if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
+ /* At runtime only non-volatile variables may be set. */
+ if (ret != EFI_INVALID_PARAMETER) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ } else {
+ if (ret != EFI_UNSUPPORTED) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
}
len = EFI_ST_MAX_DATA_SIZE;
ret = runtime->get_variable(u"PlatformLangCodes", &guid_vendor0,
--
2.40.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 2/4] efi_loader: Add OS notifications for SetVariable at runtime
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 1/4] efi_loader: conditionally enable SetvariableRT Ilias Apalodimas
@ 2024-04-18 12:54 ` Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents Ilias Apalodimas
` (2 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 12:54 UTC (permalink / raw)
To: xypron.glpk, kettenis
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Ilias Apalodimas, Heinrich Schuchardt, Tom Rini,
Masahisa Kojima, AKASHI Takahiro, Raymond Mao, Matthias Schiffer,
Janne Grunau, Simon Glass, Abdellatif El Khlifi, Sughosh Ganu,
Sam Edwards, Richard Henderson, Alper Nebi Yasak, Weizhao Ouyang,
u-boot
Previous patches enable SetVariable at runtime using a volatile storage
backend using EFI_RUNTIME_SERVICES_DATA allocared memory. Since there's
no recommendation from the spec on how to notify the OS, add a volatile
EFI variable that contains the filename relative to the ESP. OS'es
can use that file and update it at runtime
$~ efivar -p -n b2ac5fc9-92b7-4acd-aeac-11e818c3130c-RTStorageVolatile
GUID: b2ac5fc9-92b7-4acd-aeac-11e818c3130c
Name: "RTStorageVolatile"
Attributes:
Boot Service Access
Runtime Service Access
Value:
00000000 75 62 6f 6f 74 65 66 69 2e 76 61 72 00 |ubootefi.var. |
Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
include/efi_loader.h | 4 ++++
lib/efi_loader/efi_runtime.c | 19 ++++++++++++++++---
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index bb51c0281774..69442f4e58de 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -159,6 +159,10 @@ static inline void efi_set_bootdev(const char *dev, const char *devnr,
#define EFICONFIG_AUTO_GENERATED_ENTRY_GUID \
EFI_GUID(0x8108ac4e, 0x9f11, 0x4d59, \
0x85, 0x0e, 0xe2, 0x1a, 0x52, 0x2c, 0x59, 0xb2)
+#define U_BOOT_EFI_RT_VAR_FILE_GUID \
+ EFI_GUID(0xb2ac5fc9, 0x92b7, 0x4acd, \
+ 0xae, 0xac, 0x11, 0xe8, 0x18, 0xc3, 0x13, 0x0c)
+
/* Use internal device tree when starting UEFI application */
#define EFI_FDT_USE_INTERNAL NULL
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index dde083b09665..c8f7a88ba8db 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -10,6 +10,7 @@
#include <dm.h>
#include <elf.h>
#include <efi_loader.h>
+#include <efi_variable.h>
#include <log.h>
#include <malloc.h>
#include <rtc.h>
@@ -110,6 +111,7 @@ static __efi_runtime_data efi_uintn_t efi_descriptor_size;
*/
efi_status_t efi_init_runtime_supported(void)
{
+ const efi_guid_t efi_guid_efi_rt_var_file = U_BOOT_EFI_RT_VAR_FILE_GUID;
efi_status_t ret;
struct efi_rt_properties_table *rt_table;
@@ -127,9 +129,20 @@ efi_status_t efi_init_runtime_supported(void)
EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
EFI_RT_SUPPORTED_CONVERT_POINTER;
- if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
- rt_table->runtime_services_supported |=
- EFI_RT_SUPPORTED_SET_VARIABLE;
+ if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
+ ret = efi_set_variable_int(u"RTStorageVolatile",
+ &efi_guid_efi_rt_var_file,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_READ_ONLY,
+ sizeof(EFI_VAR_FILE_NAME),
+ EFI_VAR_FILE_NAME, false);
+ if (ret != EFI_SUCCESS) {
+ log_err("Failed to set RTStorageVolatile\n");
+ return ret;
+ }
+ rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
+ }
/*
* This value must be synced with efi_runtime_detach_list
--
2.40.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 1/4] efi_loader: conditionally enable SetvariableRT Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 2/4] efi_loader: Add OS notifications for SetVariable at runtime Ilias Apalodimas
@ 2024-04-18 12:54 ` Ilias Apalodimas
2024-04-18 15:15 ` Mark Kettenis
2024-04-18 12:54 ` [PATCH v3 4/4] efi_selftest: add tests for setvariableRT Ilias Apalodimas
2024-04-20 7:20 ` [PATCH v3 0/4] Heinrich Schuchardt
4 siblings, 1 reply; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 12:54 UTC (permalink / raw)
To: xypron.glpk, kettenis
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Ilias Apalodimas, Heinrich Schuchardt, Tom Rini,
Masahisa Kojima, AKASHI Takahiro, Raymond Mao, Simon Glass,
Matthias Schiffer, Janne Grunau, Abdellatif El Khlifi,
Sughosh Ganu, Richard Henderson, Sam Edwards, Alper Nebi Yasak,
Weizhao Ouyang, u-boot
Previous patches enabled SetVariableRT using a RAM backend.
Although EBBR [0] defines a variable format we can teach userspace tools
and write the altered variables, it's better if we skip the ABI
requirements completely.
So let's add a new variable, in its own namespace called "VarToFile"
which contains a binary dump of the updated RT, BS and, NV variables
and will be updated when GetVariable is called.
Some adjustments are needed to do that.
Currently we discard BS-only variables in EBS(). We need to preserve
those on the RAM backend that exposes the variables. Since BS-only
variables can't appear at runtime we need to move the memory masking
checks from efi_var_collect() to efi_get_next_variable_name_mem()/
efi_get_variable_mem() and do the filtering at runtime.
We also need an efi_var_collect() variant available at runtime, in order
to construct the "VarToFile" buffer on the fly.
All users and applications (for linux) have to do when updating a variable
is dd that variable in the file described by "RTStorageVolatile".
Linux efivarfs uses a first 4 bytes of the output to represent attributes
in little-endian format. So, storing variables works like this:
$~ efibootmgr -n 0001
$~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
[0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
include/efi_variable.h | 16 +++-
lib/charset.c | 2 +-
lib/efi_loader/efi_runtime.c | 25 +++++
lib/efi_loader/efi_var_common.c | 6 +-
lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
lib/efi_loader/efi_variable.c | 6 +-
lib/efi_loader/efi_variable_tee.c | 5 -
7 files changed, 146 insertions(+), 65 deletions(-)
diff --git a/include/efi_variable.h b/include/efi_variable.h
index 42a2b7c52bef..223bb9a4a5bd 100644
--- a/include/efi_variable.h
+++ b/include/efi_variable.h
@@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
*
* @variable_name_size: size of variable_name buffer in bytes
* @variable_name: name of uefi variable's name in u16
+ * @mask: bitmask with required attributes of variables to be collected.
+ * variables are only collected if all of the required
+ * attributes match. Use 0 to skip matching
* @vendor: vendor's guid
*
* Return: status code
*/
efi_status_t __efi_runtime
efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
- efi_guid_t *vendor);
+ efi_guid_t *vendor, u32 mask);
/**
* efi_get_variable_mem() - Runtime common code across efi variable
* implementations for GetVariable() from
@@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
* @data_size: size of the buffer to which the variable value is copied
* @data: buffer to which the variable value is copied
* @timep: authentication time (seconds since start of epoch)
+ * @mask: bitmask with required attributes of variables to be collected.
+ * variables are only collected if all of the required
+ * attributes match. Use 0 to skip matching
* Return: status code
*/
efi_status_t __efi_runtime
efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
u32 *attributes, efi_uintn_t *data_size, void *data,
- u64 *timep);
+ u64 *timep, u32 mask);
/**
* efi_get_variable_runtime() - runtime implementation of GetVariable()
@@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
*/
void efi_var_buf_update(struct efi_var_file *var_buf);
+efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
+ efi_uintn_t *lenp,
+ u32 check_attr_mask);
+
+u32 efi_var_entry_len(struct efi_var_entry *var);
+
#endif
diff --git a/lib/charset.c b/lib/charset.c
index df4f04074852..182c92a50c48 100644
--- a/lib/charset.c
+++ b/lib/charset.c
@@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
* > 0 if the first different u16 in s1 is greater than the
* corresponding u16 in s2
*/
-int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
+int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
{
int ret = 0;
diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
index c8f7a88ba8db..73831c527e00 100644
--- a/lib/efi_loader/efi_runtime.c
+++ b/lib/efi_loader/efi_runtime.c
@@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
EFI_RT_SUPPORTED_CONVERT_POINTER;
if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
+ u8 s = 0;
+
ret = efi_set_variable_int(u"RTStorageVolatile",
&efi_guid_efi_rt_var_file,
EFI_VARIABLE_BOOTSERVICE_ACCESS |
@@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
log_err("Failed to set RTStorageVolatile\n");
return ret;
}
+ /*
+ * This variable needs to be visible so users can read it,
+ * but the real contents are going to be filled during
+ * GetVariable
+ */
+ ret = efi_set_variable_int(u"VarToFile",
+ &efi_guid_efi_rt_var_file,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_READ_ONLY,
+ sizeof(s),
+ &s, false);
+ if (ret != EFI_SUCCESS) {
+ log_err("Failed to set VarToFile\n");
+ efi_set_variable_int(u"RTStorageVolatile",
+ &efi_guid_efi_rt_var_file,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_READ_ONLY,
+ 0, NULL, false);
+
+ return ret;
+ }
rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
}
diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
index aa8feffd3ec1..7862f2d6ce8a 100644
--- a/lib/efi_loader/efi_var_common.c
+++ b/lib/efi_loader/efi_var_common.c
@@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
{
efi_status_t ret;
- ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
+ ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
+ data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
/* Remove EFI_VARIABLE_READ_ONLY flag */
if (attributes)
@@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
u16 *variable_name, efi_guid_t *guid)
{
- return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
+ return efi_get_next_variable_name_mem(variable_name_size, variable_name,
+ guid, EFI_VARIABLE_RUNTIME_ACCESS);
}
/**
diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
index 6c21cec5d457..940ab6638823 100644
--- a/lib/efi_loader/efi_var_mem.c
+++ b/lib/efi_loader/efi_var_mem.c
@@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
return match;
}
+/**
+ * efi_var_entry_len() - Get the entry len including headers & name
+ *
+ * @var: pointer to variable start
+ *
+ * Return: 8-byte aligned variable entry length
+ */
+
+u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
+{
+ if (!var)
+ return 0;
+
+ return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
+ var->length + sizeof(*var), 8);
+}
+
struct efi_var_entry __efi_runtime
*efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
struct efi_var_entry **next)
@@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
sizeof(struct efi_var_entry);
}
-/**
- * efi_var_mem_bs_del() - delete boot service only variables
- */
-static void efi_var_mem_bs_del(void)
-{
- struct efi_var_entry *var = efi_var_buf->var;
-
- for (;;) {
- struct efi_var_entry *last;
-
- last = (struct efi_var_entry *)
- ((uintptr_t)efi_var_buf + efi_var_buf->length);
- if (var >= last)
- break;
- if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
- u16 *data;
-
- /* skip variable */
- for (data = var->name; *data; ++data)
- ;
- ++data;
- var = (struct efi_var_entry *)
- ALIGN((uintptr_t)data + var->length, 8);
- } else {
- /* delete variable */
- efi_var_mem_del(var);
- }
- }
-}
-
-/**
- * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
- *
- * @event: callback event
- * @context: callback context
- */
-static void EFIAPI
-efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
-{
- EFI_ENTRY("%p, %p", event, context);
-
- /* Delete boot service only variables */
- efi_var_mem_bs_del();
-
- EFI_EXIT(EFI_SUCCESS);
-}
-
/**
* efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
*
@@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
efi_var_buf->length = (uintptr_t)efi_var_buf->var -
(uintptr_t)efi_var_buf;
- /* crc32 for 0 bytes = 0 */
- ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
- efi_var_mem_notify_exit_boot_services, NULL,
- NULL, &event);
if (ret != EFI_SUCCESS)
return ret;
ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
@@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
return ret;
}
+/**
+ * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
+ * efi_var_buf
+ *
+ * @buf: buffer containing variable collection
+ * @lenp: buffer length
+ * @mask: mask of matched attributes
+ *
+ * Return: Status code
+ */
+efi_status_t __efi_runtime
+efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
+{
+ static struct efi_var_file __efi_runtime_data hdr = {
+ .magic = EFI_VAR_FILE_MAGIC,
+ };
+ struct efi_var_entry *last, *var, *var_to;
+
+ hdr.length = sizeof(struct efi_var_file);
+
+ var = efi_var_buf->var;
+ last = (struct efi_var_entry *)
+ ((uintptr_t)efi_var_buf + efi_var_buf->length);
+ if (buf)
+ var_to = buf->var;
+
+ while (var < last) {
+ u32 len = efi_var_entry_len(var);
+
+ if ((var->attr & mask) != mask) {
+ var = (void *)((uintptr_t)var + len);
+ continue;
+ }
+
+ hdr.length += len;
+
+ if (buf && hdr.length <= *lenp) {
+ efi_memcpy_runtime(var_to, var, len);
+ var_to = (void *)var_to + len;
+ }
+ var = (void *)var + len;
+ }
+
+ if (!buf && hdr.length <= *lenp) {
+ *lenp = hdr.length;
+ return EFI_INVALID_PARAMETER;
+ }
+
+ if (!buf || hdr.length > *lenp) {
+ *lenp = hdr.length;
+ return EFI_BUFFER_TOO_SMALL;
+ }
+ hdr.crc32 = crc32(0, (u8 *)buf->var,
+ hdr.length - sizeof(struct efi_var_file));
+
+ efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
+ *lenp = hdr.length;
+
+ return EFI_SUCCESS;
+}
+
efi_status_t __efi_runtime
efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
u32 *attributes, efi_uintn_t *data_size, void *data,
- u64 *timep)
+ u64 *timep, u32 mask)
{
efi_uintn_t old_size;
struct efi_var_entry *var;
@@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
if (!var)
return EFI_NOT_FOUND;
+ /*
+ * This function is used at runtime to dump EFI variables.
+ * The memory backend we keep around has BS-only variables as
+ * well. At runtime we filter them here
+ */
+ if (mask && !((var->attr & mask) == mask))
+ return EFI_NOT_FOUND;
+
if (attributes)
*attributes = var->attr;
if (timep)
*timep = var->time;
+ if (!u16_strcmp(variable_name, u"VarToFile"))
+ return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
+
old_size = *data_size;
*data_size = var->length;
if (old_size < var->length)
@@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
efi_status_t __efi_runtime
efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
- u16 *variable_name, efi_guid_t *vendor)
+ u16 *variable_name, efi_guid_t *vendor,
+ u32 mask)
{
struct efi_var_entry *var;
efi_uintn_t len, old_size;
@@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
if (!variable_name_size || !variable_name || !vendor)
return EFI_INVALID_PARAMETER;
+skip:
len = *variable_name_size >> 1;
if (u16_strnlen(variable_name, len) == len)
return EFI_INVALID_PARAMETER;
@@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
+ if (mask && !((var->attr & mask) == mask)) {
+ *variable_name_size = old_size;
+ goto skip;
+ }
+
return EFI_SUCCESS;
}
diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
index 47bb79920575..0cbed53d1dbf 100644
--- a/lib/efi_loader/efi_variable.c
+++ b/lib/efi_loader/efi_variable.c
@@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
u32 *attributes, efi_uintn_t *data_size, void *data,
u64 *timep)
{
- return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
+ return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
+ data, timep, 0);
}
efi_status_t __efi_runtime
efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
u16 *variable_name, efi_guid_t *vendor)
{
- return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
+ return efi_get_next_variable_name_mem(variable_name_size, variable_name,
+ vendor, 0);
}
/**
diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
index dde135fd9f81..4f1aa298da13 100644
--- a/lib/efi_loader/efi_variable_tee.c
+++ b/lib/efi_loader/efi_variable_tee.c
@@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
log_err("Unable to notify the MM partition for ExitBootServices\n");
free(comm_buf);
- /*
- * Populate the list for runtime variables.
- * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
- * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
- */
ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
if (ret != EFI_SUCCESS)
log_err("Can't populate EFI variables. No runtime variables will be available\n");
--
2.40.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH v3 4/4] efi_selftest: add tests for setvariableRT
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
` (2 preceding siblings ...)
2024-04-18 12:54 ` [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents Ilias Apalodimas
@ 2024-04-18 12:54 ` Ilias Apalodimas
2024-04-20 7:17 ` Heinrich Schuchardt
2024-04-20 7:20 ` [PATCH v3 0/4] Heinrich Schuchardt
4 siblings, 1 reply; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 12:54 UTC (permalink / raw)
To: xypron.glpk, kettenis
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Ilias Apalodimas, Tom Rini, Masahisa Kojima,
AKASHI Takahiro, Raymond Mao, Simon Glass, Janne Grunau,
Matthias Schiffer, Abdellatif El Khlifi, Alper Nebi Yasak,
Sughosh Ganu, Sam Edwards, Richard Henderson, Weizhao Ouyang,
u-boot
Since we support SetVariableRT now add the relevant tests
- Search for the RTStorageVolatile and VarToFile variables after EBS
- Try to update with invalid variales (BS, RT only)
- Try to write a variable bigger than our backend storage
- Write a variable that fits and check VarToFile has been updated
correclty
- Append to the variable and check VarToFile changes
- Try to delete VarToFile which is write protected
- Try to add/delete runtime variables
- Verify VarToFile contains a valid file format
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
.../efi_selftest_variables_runtime.c | 197 +++++++++++++++++-
1 file changed, 196 insertions(+), 1 deletion(-)
diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
index 986924b881dd..28b4e9e7afa5 100644
--- a/lib/efi_selftest/efi_selftest_variables_runtime.c
+++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
@@ -10,6 +10,8 @@
*/
#include <efi_selftest.h>
+#include <efi_variable.h>
+#include <u-boot/crc.h>
#define EFI_ST_MAX_DATA_SIZE 16
#define EFI_ST_MAX_VARNAME_SIZE 40
@@ -17,6 +19,8 @@
static struct efi_boot_services *boottime;
static struct efi_runtime_services *runtime;
static const efi_guid_t guid_vendor0 = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_guid_t __efi_runtime_data efi_rt_var_guid =
+ U_BOOT_EFI_RT_VAR_FILE_GUID;
/*
* Setup unit test.
@@ -41,15 +45,18 @@ static int setup(const efi_handle_t img_handle,
static int execute(void)
{
efi_status_t ret;
- efi_uintn_t len;
+ efi_uintn_t len, avail, append_len = 17;
u32 attr;
u8 v[16] = {0x5d, 0xd1, 0x5e, 0x51, 0x5a, 0x05, 0xc7, 0x0c,
0x35, 0x4a, 0xae, 0x87, 0xa5, 0xdf, 0x0f, 0x65,};
+ u8 v2[CONFIG_EFI_VAR_BUF_SIZE];
u8 data[EFI_ST_MAX_DATA_SIZE];
+ u8 data2[CONFIG_EFI_VAR_BUF_SIZE];
u16 varname[EFI_ST_MAX_VARNAME_SIZE];
efi_guid_t guid;
u64 max_storage, rem_storage, max_size;
+ memset(v2, 0x1, sizeof(v2));
ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
&max_storage, &rem_storage,
&max_size);
@@ -63,11 +70,199 @@ static int execute(void)
EFI_VARIABLE_RUNTIME_ACCESS,
3, v + 4);
if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
+ efi_uintn_t prev_len, delta;
+ struct efi_var_entry *var;
+ struct efi_var_file *hdr;
+
/* At runtime only non-volatile variables may be set. */
if (ret != EFI_INVALID_PARAMETER) {
efi_st_error("SetVariable failed\n");
return EFI_ST_FAILURE;
}
+
+ /* runtime atttribute must be set */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ 3, v + 4);
+ if (ret != EFI_INVALID_PARAMETER) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ len = sizeof(data);
+ ret = runtime->get_variable(u"RTStorageVolatile",
+ &efi_rt_var_guid,
+ &attr, &len, data);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("GetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ len = sizeof(data2);
+ ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
+ &attr, &len, data2);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("GetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ /*
+ * VarToFile size must change once a variable is inserted
+ * Store it now, we'll use it later
+ */
+ prev_len = len;
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ sizeof(v2),
+ v2);
+ /*
+ * This will try to update VarToFile as well and must fail,
+ * without changing or deleting VarToFile
+ */
+ if (ret != EFI_OUT_OF_RESOURCES) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ len = sizeof(data2);
+ ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
+ &attr, &len, data2);
+ if (ret != EFI_SUCCESS || prev_len != len) {
+ efi_st_error("Get/SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Add an 8byte aligned variable */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ sizeof(v), v);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Delete it by setting the attrs to 0 */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ 0, sizeof(v), v);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Add it back */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ sizeof(v), v);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Delete it again by setting the size to 0 */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ 0, NULL);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Delete it again and make sure it's not there */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ 0, NULL);
+ if (ret != EFI_NOT_FOUND) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /*
+ * Add a non-aligned variable
+ * VarToFile updates must include efi_st_var0
+ */
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ 9, v + 4);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ var = efi_var_mem_find(&guid_vendor0, u"efi_st_var0", NULL);
+ if (!var) {
+ efi_st_error("GetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ delta = efi_var_entry_len(var);
+ len = sizeof(data2);
+ ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
+ &attr, &len, data2);
+ if (ret != EFI_SUCCESS || prev_len + delta != len) {
+ efi_st_error("Get/SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /*
+ * Append on an existing variable must update VarToFile
+ * Our variable entries are 8-byte aligned.
+ * Adding a single byte will fit on the existing space
+ */
+ prev_len = len;
+ avail = efi_var_entry_len(var) -
+ (sizeof(u16) * (u16_strlen(var->name) + 1) + sizeof(*var)) -
+ var->length;
+ if (avail >= append_len)
+ delta = 0;
+ else
+ delta = ALIGN(append_len - avail, 8);
+ ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_APPEND_WRITE |
+ EFI_VARIABLE_NON_VOLATILE,
+ append_len, v2);
+ if (ret != EFI_SUCCESS) {
+ efi_st_error("SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+ len = sizeof(data2);
+ ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
+ &attr, &len, data2);
+ if (ret != EFI_SUCCESS || prev_len + delta != len) {
+ efi_st_error("Get/SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Make sure that variable contains a valid file */
+ hdr = (struct efi_var_file *)data2;
+ if (hdr->magic != EFI_VAR_FILE_MAGIC ||
+ len != hdr->length ||
+ hdr->crc32 != crc32(0, (u8 *)((uintptr_t)data2 + sizeof(struct efi_var_file)),
+ len - sizeof(struct efi_var_file))) {
+ efi_st_error("VarToFile invalid header\n");
+ return EFI_ST_FAILURE;
+ }
+
+ /* Variables that are BS, RT and volatile are RO after EBS */
+ ret = runtime->set_variable(u"VarToFile", &efi_rt_var_guid,
+ EFI_VARIABLE_BOOTSERVICE_ACCESS |
+ EFI_VARIABLE_RUNTIME_ACCESS |
+ EFI_VARIABLE_NON_VOLATILE,
+ sizeof(v), v);
+ if (ret != EFI_WRITE_PROTECTED) {
+ efi_st_error("Get/SetVariable failed\n");
+ return EFI_ST_FAILURE;
+ }
} else {
if (ret != EFI_UNSUPPORTED) {
efi_st_error("SetVariable failed\n");
--
2.40.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 12:54 ` [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents Ilias Apalodimas
@ 2024-04-18 15:15 ` Mark Kettenis
2024-04-18 15:36 ` Ilias Apalodimas
0 siblings, 1 reply; 15+ messages in thread
From: Mark Kettenis @ 2024-04-18 15:15 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, kettenis, caleb.connolly, sumit.garg, quic_llindhol,
ardb, pbrobinson, pjones, ilias.apalodimas, heinrich.schuchardt,
trini, kojima.masahisa, takahiro.akashi, raymond.mao, sjg,
matthias.schiffer, j, abdellatif.elkhlifi, sughosh.ganu,
richard.henderson, CFSworks, alpernebiyasak, o451686892, u-boot
> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Date: Thu, 18 Apr 2024 15:54:52 +0300
Hi Illias,
>
> Previous patches enabled SetVariableRT using a RAM backend.
> Although EBBR [0] defines a variable format we can teach userspace tools
> and write the altered variables, it's better if we skip the ABI
> requirements completely.
>
> So let's add a new variable, in its own namespace called "VarToFile"
> which contains a binary dump of the updated RT, BS and, NV variables
> and will be updated when GetVariable is called.
>
> Some adjustments are needed to do that.
> Currently we discard BS-only variables in EBS(). We need to preserve
> those on the RAM backend that exposes the variables. Since BS-only
> variables can't appear at runtime we need to move the memory masking
> checks from efi_var_collect() to efi_get_next_variable_name_mem()/
> efi_get_variable_mem() and do the filtering at runtime.
>
> We also need an efi_var_collect() variant available at runtime, in order
> to construct the "VarToFile" buffer on the fly.
>
> All users and applications (for linux) have to do when updating a variable
> is dd that variable in the file described by "RTStorageVolatile".
I simehow missed your reply to the issue I raised with picking the
right ESP to write variables back to. I'm not convinced that the ESP
that was used to install Linux on is necessarily th right one. In
particular, consider a system with multiple disks, say eMMC and an
NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
that disk to store the EFI variables. Now I install Linux on the NVMe
disk, which creates an ESP to store its bootloader. With your
proposed changes, Linux will probably end up writing the variables to
the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
variables from eMMC and you've lost any changes...
> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> in little-endian format. So, storing variables works like this:
>
> $~ efibootmgr -n 0001
> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
>
> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>
> Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> include/efi_variable.h | 16 +++-
> lib/charset.c | 2 +-
> lib/efi_loader/efi_runtime.c | 25 +++++
> lib/efi_loader/efi_var_common.c | 6 +-
> lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
> lib/efi_loader/efi_variable.c | 6 +-
> lib/efi_loader/efi_variable_tee.c | 5 -
> 7 files changed, 146 insertions(+), 65 deletions(-)
>
> diff --git a/include/efi_variable.h b/include/efi_variable.h
> index 42a2b7c52bef..223bb9a4a5bd 100644
> --- a/include/efi_variable.h
> +++ b/include/efi_variable.h
> @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
> *
> * @variable_name_size: size of variable_name buffer in bytes
> * @variable_name: name of uefi variable's name in u16
> + * @mask: bitmask with required attributes of variables to be collected.
> + * variables are only collected if all of the required
> + * attributes match. Use 0 to skip matching
> * @vendor: vendor's guid
> *
> * Return: status code
> */
> efi_status_t __efi_runtime
> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> - efi_guid_t *vendor);
> + efi_guid_t *vendor, u32 mask);
> /**
> * efi_get_variable_mem() - Runtime common code across efi variable
> * implementations for GetVariable() from
> @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
> * @data_size: size of the buffer to which the variable value is copied
> * @data: buffer to which the variable value is copied
> * @timep: authentication time (seconds since start of epoch)
> + * @mask: bitmask with required attributes of variables to be collected.
> + * variables are only collected if all of the required
> + * attributes match. Use 0 to skip matching
> * Return: status code
> */
> efi_status_t __efi_runtime
> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> u32 *attributes, efi_uintn_t *data_size, void *data,
> - u64 *timep);
> + u64 *timep, u32 mask);
>
> /**
> * efi_get_variable_runtime() - runtime implementation of GetVariable()
> @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> */
> void efi_var_buf_update(struct efi_var_file *var_buf);
>
> +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
> + efi_uintn_t *lenp,
> + u32 check_attr_mask);
> +
> +u32 efi_var_entry_len(struct efi_var_entry *var);
> +
> #endif
> diff --git a/lib/charset.c b/lib/charset.c
> index df4f04074852..182c92a50c48 100644
> --- a/lib/charset.c
> +++ b/lib/charset.c
> @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
> * > 0 if the first different u16 in s1 is greater than the
> * corresponding u16 in s2
> */
> -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> {
> int ret = 0;
>
> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> index c8f7a88ba8db..73831c527e00 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
> EFI_RT_SUPPORTED_CONVERT_POINTER;
>
> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> + u8 s = 0;
> +
> ret = efi_set_variable_int(u"RTStorageVolatile",
> &efi_guid_efi_rt_var_file,
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
> log_err("Failed to set RTStorageVolatile\n");
> return ret;
> }
> + /*
> + * This variable needs to be visible so users can read it,
> + * but the real contents are going to be filled during
> + * GetVariable
> + */
> + ret = efi_set_variable_int(u"VarToFile",
> + &efi_guid_efi_rt_var_file,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_READ_ONLY,
> + sizeof(s),
> + &s, false);
> + if (ret != EFI_SUCCESS) {
> + log_err("Failed to set VarToFile\n");
> + efi_set_variable_int(u"RTStorageVolatile",
> + &efi_guid_efi_rt_var_file,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_READ_ONLY,
> + 0, NULL, false);
> +
> + return ret;
> + }
> rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
> }
>
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index aa8feffd3ec1..7862f2d6ce8a 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> {
> efi_status_t ret;
>
> - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
> + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
>
> /* Remove EFI_VARIABLE_READ_ONLY flag */
> if (attributes)
> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> u16 *variable_name, efi_guid_t *guid)
> {
> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> + guid, EFI_VARIABLE_RUNTIME_ACCESS);
> }
>
> /**
> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> index 6c21cec5d457..940ab6638823 100644
> --- a/lib/efi_loader/efi_var_mem.c
> +++ b/lib/efi_loader/efi_var_mem.c
> @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
> return match;
> }
>
> +/**
> + * efi_var_entry_len() - Get the entry len including headers & name
> + *
> + * @var: pointer to variable start
> + *
> + * Return: 8-byte aligned variable entry length
> + */
> +
> +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
> +{
> + if (!var)
> + return 0;
> +
> + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
> + var->length + sizeof(*var), 8);
> +}
> +
> struct efi_var_entry __efi_runtime
> *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
> struct efi_var_entry **next)
> @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
> sizeof(struct efi_var_entry);
> }
>
> -/**
> - * efi_var_mem_bs_del() - delete boot service only variables
> - */
> -static void efi_var_mem_bs_del(void)
> -{
> - struct efi_var_entry *var = efi_var_buf->var;
> -
> - for (;;) {
> - struct efi_var_entry *last;
> -
> - last = (struct efi_var_entry *)
> - ((uintptr_t)efi_var_buf + efi_var_buf->length);
> - if (var >= last)
> - break;
> - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> - u16 *data;
> -
> - /* skip variable */
> - for (data = var->name; *data; ++data)
> - ;
> - ++data;
> - var = (struct efi_var_entry *)
> - ALIGN((uintptr_t)data + var->length, 8);
> - } else {
> - /* delete variable */
> - efi_var_mem_del(var);
> - }
> - }
> -}
> -
> -/**
> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> - *
> - * @event: callback event
> - * @context: callback context
> - */
> -static void EFIAPI
> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> -{
> - EFI_ENTRY("%p, %p", event, context);
> -
> - /* Delete boot service only variables */
> - efi_var_mem_bs_del();
> -
> - EFI_EXIT(EFI_SUCCESS);
> -}
> -
> /**
> * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
> *
> @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
> efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> efi_var_buf->length = (uintptr_t)efi_var_buf->var -
> (uintptr_t)efi_var_buf;
> - /* crc32 for 0 bytes = 0 */
>
> - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> - efi_var_mem_notify_exit_boot_services, NULL,
> - NULL, &event);
> if (ret != EFI_SUCCESS)
> return ret;
> ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
> @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
> return ret;
> }
>
> +/**
> + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
> + * efi_var_buf
> + *
> + * @buf: buffer containing variable collection
> + * @lenp: buffer length
> + * @mask: mask of matched attributes
> + *
> + * Return: Status code
> + */
> +efi_status_t __efi_runtime
> +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
> +{
> + static struct efi_var_file __efi_runtime_data hdr = {
> + .magic = EFI_VAR_FILE_MAGIC,
> + };
> + struct efi_var_entry *last, *var, *var_to;
> +
> + hdr.length = sizeof(struct efi_var_file);
> +
> + var = efi_var_buf->var;
> + last = (struct efi_var_entry *)
> + ((uintptr_t)efi_var_buf + efi_var_buf->length);
> + if (buf)
> + var_to = buf->var;
> +
> + while (var < last) {
> + u32 len = efi_var_entry_len(var);
> +
> + if ((var->attr & mask) != mask) {
> + var = (void *)((uintptr_t)var + len);
> + continue;
> + }
> +
> + hdr.length += len;
> +
> + if (buf && hdr.length <= *lenp) {
> + efi_memcpy_runtime(var_to, var, len);
> + var_to = (void *)var_to + len;
> + }
> + var = (void *)var + len;
> + }
> +
> + if (!buf && hdr.length <= *lenp) {
> + *lenp = hdr.length;
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + if (!buf || hdr.length > *lenp) {
> + *lenp = hdr.length;
> + return EFI_BUFFER_TOO_SMALL;
> + }
> + hdr.crc32 = crc32(0, (u8 *)buf->var,
> + hdr.length - sizeof(struct efi_var_file));
> +
> + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
> + *lenp = hdr.length;
> +
> + return EFI_SUCCESS;
> +}
> +
> efi_status_t __efi_runtime
> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> u32 *attributes, efi_uintn_t *data_size, void *data,
> - u64 *timep)
> + u64 *timep, u32 mask)
> {
> efi_uintn_t old_size;
> struct efi_var_entry *var;
> @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> if (!var)
> return EFI_NOT_FOUND;
>
> + /*
> + * This function is used at runtime to dump EFI variables.
> + * The memory backend we keep around has BS-only variables as
> + * well. At runtime we filter them here
> + */
> + if (mask && !((var->attr & mask) == mask))
> + return EFI_NOT_FOUND;
> +
> if (attributes)
> *attributes = var->attr;
> if (timep)
> *timep = var->time;
>
> + if (!u16_strcmp(variable_name, u"VarToFile"))
> + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
> +
> old_size = *data_size;
> *data_size = var->length;
> if (old_size < var->length)
> @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>
> efi_status_t __efi_runtime
> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> - u16 *variable_name, efi_guid_t *vendor)
> + u16 *variable_name, efi_guid_t *vendor,
> + u32 mask)
> {
> struct efi_var_entry *var;
> efi_uintn_t len, old_size;
> @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> if (!variable_name_size || !variable_name || !vendor)
> return EFI_INVALID_PARAMETER;
>
> +skip:
> len = *variable_name_size >> 1;
> if (u16_strnlen(variable_name, len) == len)
> return EFI_INVALID_PARAMETER;
> @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
>
> + if (mask && !((var->attr & mask) == mask)) {
> + *variable_name_size = old_size;
> + goto skip;
> + }
> +
> return EFI_SUCCESS;
> }
>
> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> index 47bb79920575..0cbed53d1dbf 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
> u32 *attributes, efi_uintn_t *data_size, void *data,
> u64 *timep)
> {
> - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
> + data, timep, 0);
> }
>
> efi_status_t __efi_runtime
> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> u16 *variable_name, efi_guid_t *vendor)
> {
> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> + vendor, 0);
> }
>
> /**
> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> index dde135fd9f81..4f1aa298da13 100644
> --- a/lib/efi_loader/efi_variable_tee.c
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
> log_err("Unable to notify the MM partition for ExitBootServices\n");
> free(comm_buf);
>
> - /*
> - * Populate the list for runtime variables.
> - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
> - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
> - */
> ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
> if (ret != EFI_SUCCESS)
> log_err("Can't populate EFI variables. No runtime variables will be available\n");
> --
> 2.40.1
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 15:15 ` Mark Kettenis
@ 2024-04-18 15:36 ` Ilias Apalodimas
2024-04-18 15:42 ` Heinrich Schuchardt
0 siblings, 1 reply; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 15:36 UTC (permalink / raw)
To: Mark Kettenis
Cc: xypron.glpk, kettenis, caleb.connolly, sumit.garg, quic_llindhol,
ardb, pbrobinson, pjones, heinrich.schuchardt, trini,
kojima.masahisa, takahiro.akashi, raymond.mao, sjg,
matthias.schiffer, j, abdellatif.elkhlifi, sughosh.ganu,
richard.henderson, CFSworks, alpernebiyasak, o451686892, u-boot
Hi Mark,
On Thu, 18 Apr 2024 at 18:15, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>
> > From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Date: Thu, 18 Apr 2024 15:54:52 +0300
>
> Hi Illias,
>
> >
> > Previous patches enabled SetVariableRT using a RAM backend.
> > Although EBBR [0] defines a variable format we can teach userspace tools
> > and write the altered variables, it's better if we skip the ABI
> > requirements completely.
> >
> > So let's add a new variable, in its own namespace called "VarToFile"
> > which contains a binary dump of the updated RT, BS and, NV variables
> > and will be updated when GetVariable is called.
> >
> > Some adjustments are needed to do that.
> > Currently we discard BS-only variables in EBS(). We need to preserve
> > those on the RAM backend that exposes the variables. Since BS-only
> > variables can't appear at runtime we need to move the memory masking
> > checks from efi_var_collect() to efi_get_next_variable_name_mem()/
> > efi_get_variable_mem() and do the filtering at runtime.
> >
> > We also need an efi_var_collect() variant available at runtime, in order
> > to construct the "VarToFile" buffer on the fly.
> >
> > All users and applications (for linux) have to do when updating a variable
> > is dd that variable in the file described by "RTStorageVolatile".
>
> I simehow missed your reply to the issue I raised with picking the
> right ESP to write variables back to.
No worries, I did send v3 quickly myself...
> I'm not convinced that the ESP
> that was used to install Linux on is necessarily th right one. In
> particular, consider a system with multiple disks, say eMMC and an
> NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
> that disk to store the EFI variables.
And who creates ESP on the eMMC? I assume you have one OS installed in
the eMMC and another one in the nvme?
> Now I install Linux on the NVMe
> disk, which creates an ESP to store its bootloader. With your
> proposed changes, Linux will probably end up writing the variables to
> the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
> variables from eMMC and you've lost any changes...
I understand the problem, but my concern is that using a DP just
delegates the problem -- it doesn't solve it.
To use the 'correct' partition, U-Boot has to *decide* which ESP is
going to be used and inject it in RTVolatileStorage.
But if U-Boot decides about the 'correct' ESP the relative path would
work as well. So what's needed here is a mechanism to correlate the
booted OS with the ESP it will use and force the variable loading from
that file. Am I missing anything?
/Ilias
>
> > Linux efivarfs uses a first 4 bytes of the output to represent attributes
> > in little-endian format. So, storing variables works like this:
> >
> > $~ efibootmgr -n 0001
> > $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
> >
> > [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >
> > Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> > Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > include/efi_variable.h | 16 +++-
> > lib/charset.c | 2 +-
> > lib/efi_loader/efi_runtime.c | 25 +++++
> > lib/efi_loader/efi_var_common.c | 6 +-
> > lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
> > lib/efi_loader/efi_variable.c | 6 +-
> > lib/efi_loader/efi_variable_tee.c | 5 -
> > 7 files changed, 146 insertions(+), 65 deletions(-)
> >
> > diff --git a/include/efi_variable.h b/include/efi_variable.h
> > index 42a2b7c52bef..223bb9a4a5bd 100644
> > --- a/include/efi_variable.h
> > +++ b/include/efi_variable.h
> > @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
> > *
> > * @variable_name_size: size of variable_name buffer in bytes
> > * @variable_name: name of uefi variable's name in u16
> > + * @mask: bitmask with required attributes of variables to be collected.
> > + * variables are only collected if all of the required
> > + * attributes match. Use 0 to skip matching
> > * @vendor: vendor's guid
> > *
> > * Return: status code
> > */
> > efi_status_t __efi_runtime
> > efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> > - efi_guid_t *vendor);
> > + efi_guid_t *vendor, u32 mask);
> > /**
> > * efi_get_variable_mem() - Runtime common code across efi variable
> > * implementations for GetVariable() from
> > @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
> > * @data_size: size of the buffer to which the variable value is copied
> > * @data: buffer to which the variable value is copied
> > * @timep: authentication time (seconds since start of epoch)
> > + * @mask: bitmask with required attributes of variables to be collected.
> > + * variables are only collected if all of the required
> > + * attributes match. Use 0 to skip matching
> > * Return: status code
> > */
> > efi_status_t __efi_runtime
> > efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> > u32 *attributes, efi_uintn_t *data_size, void *data,
> > - u64 *timep);
> > + u64 *timep, u32 mask);
> >
> > /**
> > * efi_get_variable_runtime() - runtime implementation of GetVariable()
> > @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> > */
> > void efi_var_buf_update(struct efi_var_file *var_buf);
> >
> > +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
> > + efi_uintn_t *lenp,
> > + u32 check_attr_mask);
> > +
> > +u32 efi_var_entry_len(struct efi_var_entry *var);
> > +
> > #endif
> > diff --git a/lib/charset.c b/lib/charset.c
> > index df4f04074852..182c92a50c48 100644
> > --- a/lib/charset.c
> > +++ b/lib/charset.c
> > @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
> > * > 0 if the first different u16 in s1 is greater than the
> > * corresponding u16 in s2
> > */
> > -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> > +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> > {
> > int ret = 0;
> >
> > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> > index c8f7a88ba8db..73831c527e00 100644
> > --- a/lib/efi_loader/efi_runtime.c
> > +++ b/lib/efi_loader/efi_runtime.c
> > @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
> > EFI_RT_SUPPORTED_CONVERT_POINTER;
> >
> > if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > + u8 s = 0;
> > +
> > ret = efi_set_variable_int(u"RTStorageVolatile",
> > &efi_guid_efi_rt_var_file,
> > EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
> > log_err("Failed to set RTStorageVolatile\n");
> > return ret;
> > }
> > + /*
> > + * This variable needs to be visible so users can read it,
> > + * but the real contents are going to be filled during
> > + * GetVariable
> > + */
> > + ret = efi_set_variable_int(u"VarToFile",
> > + &efi_guid_efi_rt_var_file,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_READ_ONLY,
> > + sizeof(s),
> > + &s, false);
> > + if (ret != EFI_SUCCESS) {
> > + log_err("Failed to set VarToFile\n");
> > + efi_set_variable_int(u"RTStorageVolatile",
> > + &efi_guid_efi_rt_var_file,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_READ_ONLY,
> > + 0, NULL, false);
> > +
> > + return ret;
> > + }
> > rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
> > }
> >
> > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > index aa8feffd3ec1..7862f2d6ce8a 100644
> > --- a/lib/efi_loader/efi_var_common.c
> > +++ b/lib/efi_loader/efi_var_common.c
> > @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> > {
> > efi_status_t ret;
> >
> > - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> > + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
> > + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
> >
> > /* Remove EFI_VARIABLE_READ_ONLY flag */
> > if (attributes)
> > @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
> > efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> > u16 *variable_name, efi_guid_t *guid)
> > {
> > - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> > + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> > + guid, EFI_VARIABLE_RUNTIME_ACCESS);
> > }
> >
> > /**
> > diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> > index 6c21cec5d457..940ab6638823 100644
> > --- a/lib/efi_loader/efi_var_mem.c
> > +++ b/lib/efi_loader/efi_var_mem.c
> > @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
> > return match;
> > }
> >
> > +/**
> > + * efi_var_entry_len() - Get the entry len including headers & name
> > + *
> > + * @var: pointer to variable start
> > + *
> > + * Return: 8-byte aligned variable entry length
> > + */
> > +
> > +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
> > +{
> > + if (!var)
> > + return 0;
> > +
> > + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
> > + var->length + sizeof(*var), 8);
> > +}
> > +
> > struct efi_var_entry __efi_runtime
> > *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
> > struct efi_var_entry **next)
> > @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
> > sizeof(struct efi_var_entry);
> > }
> >
> > -/**
> > - * efi_var_mem_bs_del() - delete boot service only variables
> > - */
> > -static void efi_var_mem_bs_del(void)
> > -{
> > - struct efi_var_entry *var = efi_var_buf->var;
> > -
> > - for (;;) {
> > - struct efi_var_entry *last;
> > -
> > - last = (struct efi_var_entry *)
> > - ((uintptr_t)efi_var_buf + efi_var_buf->length);
> > - if (var >= last)
> > - break;
> > - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> > - u16 *data;
> > -
> > - /* skip variable */
> > - for (data = var->name; *data; ++data)
> > - ;
> > - ++data;
> > - var = (struct efi_var_entry *)
> > - ALIGN((uintptr_t)data + var->length, 8);
> > - } else {
> > - /* delete variable */
> > - efi_var_mem_del(var);
> > - }
> > - }
> > -}
> > -
> > -/**
> > - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> > - *
> > - * @event: callback event
> > - * @context: callback context
> > - */
> > -static void EFIAPI
> > -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> > -{
> > - EFI_ENTRY("%p, %p", event, context);
> > -
> > - /* Delete boot service only variables */
> > - efi_var_mem_bs_del();
> > -
> > - EFI_EXIT(EFI_SUCCESS);
> > -}
> > -
> > /**
> > * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
> > *
> > @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
> > efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> > efi_var_buf->length = (uintptr_t)efi_var_buf->var -
> > (uintptr_t)efi_var_buf;
> > - /* crc32 for 0 bytes = 0 */
> >
> > - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> > - efi_var_mem_notify_exit_boot_services, NULL,
> > - NULL, &event);
> > if (ret != EFI_SUCCESS)
> > return ret;
> > ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
> > @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
> > return ret;
> > }
> >
> > +/**
> > + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
> > + * efi_var_buf
> > + *
> > + * @buf: buffer containing variable collection
> > + * @lenp: buffer length
> > + * @mask: mask of matched attributes
> > + *
> > + * Return: Status code
> > + */
> > +efi_status_t __efi_runtime
> > +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
> > +{
> > + static struct efi_var_file __efi_runtime_data hdr = {
> > + .magic = EFI_VAR_FILE_MAGIC,
> > + };
> > + struct efi_var_entry *last, *var, *var_to;
> > +
> > + hdr.length = sizeof(struct efi_var_file);
> > +
> > + var = efi_var_buf->var;
> > + last = (struct efi_var_entry *)
> > + ((uintptr_t)efi_var_buf + efi_var_buf->length);
> > + if (buf)
> > + var_to = buf->var;
> > +
> > + while (var < last) {
> > + u32 len = efi_var_entry_len(var);
> > +
> > + if ((var->attr & mask) != mask) {
> > + var = (void *)((uintptr_t)var + len);
> > + continue;
> > + }
> > +
> > + hdr.length += len;
> > +
> > + if (buf && hdr.length <= *lenp) {
> > + efi_memcpy_runtime(var_to, var, len);
> > + var_to = (void *)var_to + len;
> > + }
> > + var = (void *)var + len;
> > + }
> > +
> > + if (!buf && hdr.length <= *lenp) {
> > + *lenp = hdr.length;
> > + return EFI_INVALID_PARAMETER;
> > + }
> > +
> > + if (!buf || hdr.length > *lenp) {
> > + *lenp = hdr.length;
> > + return EFI_BUFFER_TOO_SMALL;
> > + }
> > + hdr.crc32 = crc32(0, (u8 *)buf->var,
> > + hdr.length - sizeof(struct efi_var_file));
> > +
> > + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
> > + *lenp = hdr.length;
> > +
> > + return EFI_SUCCESS;
> > +}
> > +
> > efi_status_t __efi_runtime
> > efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> > u32 *attributes, efi_uintn_t *data_size, void *data,
> > - u64 *timep)
> > + u64 *timep, u32 mask)
> > {
> > efi_uintn_t old_size;
> > struct efi_var_entry *var;
> > @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> > if (!var)
> > return EFI_NOT_FOUND;
> >
> > + /*
> > + * This function is used at runtime to dump EFI variables.
> > + * The memory backend we keep around has BS-only variables as
> > + * well. At runtime we filter them here
> > + */
> > + if (mask && !((var->attr & mask) == mask))
> > + return EFI_NOT_FOUND;
> > +
> > if (attributes)
> > *attributes = var->attr;
> > if (timep)
> > *timep = var->time;
> >
> > + if (!u16_strcmp(variable_name, u"VarToFile"))
> > + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
> > +
> > old_size = *data_size;
> > *data_size = var->length;
> > if (old_size < var->length)
> > @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >
> > efi_status_t __efi_runtime
> > efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> > - u16 *variable_name, efi_guid_t *vendor)
> > + u16 *variable_name, efi_guid_t *vendor,
> > + u32 mask)
> > {
> > struct efi_var_entry *var;
> > efi_uintn_t len, old_size;
> > @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> > if (!variable_name_size || !variable_name || !vendor)
> > return EFI_INVALID_PARAMETER;
> >
> > +skip:
> > len = *variable_name_size >> 1;
> > if (u16_strnlen(variable_name, len) == len)
> > return EFI_INVALID_PARAMETER;
> > @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> > efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> > efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
> >
> > + if (mask && !((var->attr & mask) == mask)) {
> > + *variable_name_size = old_size;
> > + goto skip;
> > + }
> > +
> > return EFI_SUCCESS;
> > }
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 47bb79920575..0cbed53d1dbf 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
> > u32 *attributes, efi_uintn_t *data_size, void *data,
> > u64 *timep)
> > {
> > - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> > + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
> > + data, timep, 0);
> > }
> >
> > efi_status_t __efi_runtime
> > efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> > u16 *variable_name, efi_guid_t *vendor)
> > {
> > - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> > + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> > + vendor, 0);
> > }
> >
> > /**
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index dde135fd9f81..4f1aa298da13 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
> > log_err("Unable to notify the MM partition for ExitBootServices\n");
> > free(comm_buf);
> >
> > - /*
> > - * Populate the list for runtime variables.
> > - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
> > - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
> > - */
> > ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
> > if (ret != EFI_SUCCESS)
> > log_err("Can't populate EFI variables. No runtime variables will be available\n");
> > --
> > 2.40.1
> >
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 15:36 ` Ilias Apalodimas
@ 2024-04-18 15:42 ` Heinrich Schuchardt
2024-04-18 15:59 ` Ilias Apalodimas
0 siblings, 1 reply; 15+ messages in thread
From: Heinrich Schuchardt @ 2024-04-18 15:42 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: xypron.glpk, kettenis, caleb.connolly, sumit.garg, quic_llindhol,
ardb, pbrobinson, pjones, trini, kojima.masahisa,
takahiro.akashi, raymond.mao, sjg, matthias.schiffer, j,
abdellatif.elkhlifi, sughosh.ganu, richard.henderson, CFSworks,
alpernebiyasak, o451686892, u-boot, Mark Kettenis
On 18.04.24 17:36, Ilias Apalodimas wrote:
> Hi Mark,
>
> On Thu, 18 Apr 2024 at 18:15, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>
>>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> Date: Thu, 18 Apr 2024 15:54:52 +0300
>>
>> Hi Illias,
>>
>>>
>>> Previous patches enabled SetVariableRT using a RAM backend.
>>> Although EBBR [0] defines a variable format we can teach userspace tools
>>> and write the altered variables, it's better if we skip the ABI
>>> requirements completely.
>>>
>>> So let's add a new variable, in its own namespace called "VarToFile"
>>> which contains a binary dump of the updated RT, BS and, NV variables
>>> and will be updated when GetVariable is called.
>>>
>>> Some adjustments are needed to do that.
>>> Currently we discard BS-only variables in EBS(). We need to preserve
>>> those on the RAM backend that exposes the variables. Since BS-only
>>> variables can't appear at runtime we need to move the memory masking
>>> checks from efi_var_collect() to efi_get_next_variable_name_mem()/
>>> efi_get_variable_mem() and do the filtering at runtime.
>>>
>>> We also need an efi_var_collect() variant available at runtime, in order
>>> to construct the "VarToFile" buffer on the fly.
>>>
>>> All users and applications (for linux) have to do when updating a variable
>>> is dd that variable in the file described by "RTStorageVolatile".
>>
>> I simehow missed your reply to the issue I raised with picking the
>> right ESP to write variables back to.
>
> No worries, I did send v3 quickly myself...
>
>> I'm not convinced that the ESP
>> that was used to install Linux on is necessarily th right one. In
>> particular, consider a system with multiple disks, say eMMC and an
>> NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
>> that disk to store the EFI variables.
>
> And who creates ESP on the eMMC? I assume you have one OS installed in
> the eMMC and another one in the nvme?
E.g. you have a naked NVMe. At first boot there will be an ESP on the
installation medium. The installer may create an ESP on the NVMe drive
and put GRUB there.
Later the user will typically remove the installation module if U-Boot
is on SPI flash.
U-Boot has no chance to know this beforehand.
Best regards
Heinrich
>
>> Now I install Linux on the NVMe
>> disk, which creates an ESP to store its bootloader. With your
>> proposed changes, Linux will probably end up writing the variables to
>> the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
>> variables from eMMC and you've lost any changes...
>
> I understand the problem, but my concern is that using a DP just
> delegates the problem -- it doesn't solve it.
>
> To use the 'correct' partition, U-Boot has to *decide* which ESP is
> going to be used and inject it in RTVolatileStorage.
> But if U-Boot decides about the 'correct' ESP the relative path would
> work as well. So what's needed here is a mechanism to correlate the
> booted OS with the ESP it will use and force the variable loading from
> that file. Am I missing anything?
>
> /Ilias
>>
>>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
>>> in little-endian format. So, storing variables works like this:
>>>
>>> $~ efibootmgr -n 0001
>>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
>>>
>>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>>>
>>> Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
>>> Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>> ---
>>> include/efi_variable.h | 16 +++-
>>> lib/charset.c | 2 +-
>>> lib/efi_loader/efi_runtime.c | 25 +++++
>>> lib/efi_loader/efi_var_common.c | 6 +-
>>> lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
>>> lib/efi_loader/efi_variable.c | 6 +-
>>> lib/efi_loader/efi_variable_tee.c | 5 -
>>> 7 files changed, 146 insertions(+), 65 deletions(-)
>>>
>>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>>> index 42a2b7c52bef..223bb9a4a5bd 100644
>>> --- a/include/efi_variable.h
>>> +++ b/include/efi_variable.h
>>> @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
>>> *
>>> * @variable_name_size: size of variable_name buffer in bytes
>>> * @variable_name: name of uefi variable's name in u16
>>> + * @mask: bitmask with required attributes of variables to be collected.
>>> + * variables are only collected if all of the required
>>> + * attributes match. Use 0 to skip matching
>>> * @vendor: vendor's guid
>>> *
>>> * Return: status code
>>> */
>>> efi_status_t __efi_runtime
>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
>>> - efi_guid_t *vendor);
>>> + efi_guid_t *vendor, u32 mask);
>>> /**
>>> * efi_get_variable_mem() - Runtime common code across efi variable
>>> * implementations for GetVariable() from
>>> @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
>>> * @data_size: size of the buffer to which the variable value is copied
>>> * @data: buffer to which the variable value is copied
>>> * @timep: authentication time (seconds since start of epoch)
>>> + * @mask: bitmask with required attributes of variables to be collected.
>>> + * variables are only collected if all of the required
>>> + * attributes match. Use 0 to skip matching
>>> * Return: status code
>>> */
>>> efi_status_t __efi_runtime
>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>> - u64 *timep);
>>> + u64 *timep, u32 mask);
>>>
>>> /**
>>> * efi_get_variable_runtime() - runtime implementation of GetVariable()
>>> @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>>> */
>>> void efi_var_buf_update(struct efi_var_file *var_buf);
>>>
>>> +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
>>> + efi_uintn_t *lenp,
>>> + u32 check_attr_mask);
>>> +
>>> +u32 efi_var_entry_len(struct efi_var_entry *var);
>>> +
>>> #endif
>>> diff --git a/lib/charset.c b/lib/charset.c
>>> index df4f04074852..182c92a50c48 100644
>>> --- a/lib/charset.c
>>> +++ b/lib/charset.c
>>> @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
>>> * > 0 if the first different u16 in s1 is greater than the
>>> * corresponding u16 in s2
>>> */
>>> -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
>>> +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
>>> {
>>> int ret = 0;
>>>
>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>> index c8f7a88ba8db..73831c527e00 100644
>>> --- a/lib/efi_loader/efi_runtime.c
>>> +++ b/lib/efi_loader/efi_runtime.c
>>> @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
>>> EFI_RT_SUPPORTED_CONVERT_POINTER;
>>>
>>> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
>>> + u8 s = 0;
>>> +
>>> ret = efi_set_variable_int(u"RTStorageVolatile",
>>> &efi_guid_efi_rt_var_file,
>>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
>>> log_err("Failed to set RTStorageVolatile\n");
>>> return ret;
>>> }
>>> + /*
>>> + * This variable needs to be visible so users can read it,
>>> + * but the real contents are going to be filled during
>>> + * GetVariable
>>> + */
>>> + ret = efi_set_variable_int(u"VarToFile",
>>> + &efi_guid_efi_rt_var_file,
>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> + EFI_VARIABLE_RUNTIME_ACCESS |
>>> + EFI_VARIABLE_READ_ONLY,
>>> + sizeof(s),
>>> + &s, false);
>>> + if (ret != EFI_SUCCESS) {
>>> + log_err("Failed to set VarToFile\n");
>>> + efi_set_variable_int(u"RTStorageVolatile",
>>> + &efi_guid_efi_rt_var_file,
>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>> + EFI_VARIABLE_RUNTIME_ACCESS |
>>> + EFI_VARIABLE_READ_ONLY,
>>> + 0, NULL, false);
>>> +
>>> + return ret;
>>> + }
>>> rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
>>> }
>>>
>>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>>> index aa8feffd3ec1..7862f2d6ce8a 100644
>>> --- a/lib/efi_loader/efi_var_common.c
>>> +++ b/lib/efi_loader/efi_var_common.c
>>> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
>>> {
>>> efi_status_t ret;
>>>
>>> - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
>>> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
>>> + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
>>>
>>> /* Remove EFI_VARIABLE_READ_ONLY flag */
>>> if (attributes)
>>> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
>>> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>>> u16 *variable_name, efi_guid_t *guid)
>>> {
>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
>>> + guid, EFI_VARIABLE_RUNTIME_ACCESS);
>>> }
>>>
>>> /**
>>> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
>>> index 6c21cec5d457..940ab6638823 100644
>>> --- a/lib/efi_loader/efi_var_mem.c
>>> +++ b/lib/efi_loader/efi_var_mem.c
>>> @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
>>> return match;
>>> }
>>>
>>> +/**
>>> + * efi_var_entry_len() - Get the entry len including headers & name
>>> + *
>>> + * @var: pointer to variable start
>>> + *
>>> + * Return: 8-byte aligned variable entry length
>>> + */
>>> +
>>> +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
>>> +{
>>> + if (!var)
>>> + return 0;
>>> +
>>> + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
>>> + var->length + sizeof(*var), 8);
>>> +}
>>> +
>>> struct efi_var_entry __efi_runtime
>>> *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
>>> struct efi_var_entry **next)
>>> @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
>>> sizeof(struct efi_var_entry);
>>> }
>>>
>>> -/**
>>> - * efi_var_mem_bs_del() - delete boot service only variables
>>> - */
>>> -static void efi_var_mem_bs_del(void)
>>> -{
>>> - struct efi_var_entry *var = efi_var_buf->var;
>>> -
>>> - for (;;) {
>>> - struct efi_var_entry *last;
>>> -
>>> - last = (struct efi_var_entry *)
>>> - ((uintptr_t)efi_var_buf + efi_var_buf->length);
>>> - if (var >= last)
>>> - break;
>>> - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
>>> - u16 *data;
>>> -
>>> - /* skip variable */
>>> - for (data = var->name; *data; ++data)
>>> - ;
>>> - ++data;
>>> - var = (struct efi_var_entry *)
>>> - ALIGN((uintptr_t)data + var->length, 8);
>>> - } else {
>>> - /* delete variable */
>>> - efi_var_mem_del(var);
>>> - }
>>> - }
>>> -}
>>> -
>>> -/**
>>> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
>>> - *
>>> - * @event: callback event
>>> - * @context: callback context
>>> - */
>>> -static void EFIAPI
>>> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
>>> -{
>>> - EFI_ENTRY("%p, %p", event, context);
>>> -
>>> - /* Delete boot service only variables */
>>> - efi_var_mem_bs_del();
>>> -
>>> - EFI_EXIT(EFI_SUCCESS);
>>> -}
>>> -
>>> /**
>>> * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
>>> *
>>> @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
>>> efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
>>> efi_var_buf->length = (uintptr_t)efi_var_buf->var -
>>> (uintptr_t)efi_var_buf;
>>> - /* crc32 for 0 bytes = 0 */
>>>
>>> - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
>>> - efi_var_mem_notify_exit_boot_services, NULL,
>>> - NULL, &event);
>>> if (ret != EFI_SUCCESS)
>>> return ret;
>>> ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
>>> @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
>>> return ret;
>>> }
>>>
>>> +/**
>>> + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
>>> + * efi_var_buf
>>> + *
>>> + * @buf: buffer containing variable collection
>>> + * @lenp: buffer length
>>> + * @mask: mask of matched attributes
>>> + *
>>> + * Return: Status code
>>> + */
>>> +efi_status_t __efi_runtime
>>> +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
>>> +{
>>> + static struct efi_var_file __efi_runtime_data hdr = {
>>> + .magic = EFI_VAR_FILE_MAGIC,
>>> + };
>>> + struct efi_var_entry *last, *var, *var_to;
>>> +
>>> + hdr.length = sizeof(struct efi_var_file);
>>> +
>>> + var = efi_var_buf->var;
>>> + last = (struct efi_var_entry *)
>>> + ((uintptr_t)efi_var_buf + efi_var_buf->length);
>>> + if (buf)
>>> + var_to = buf->var;
>>> +
>>> + while (var < last) {
>>> + u32 len = efi_var_entry_len(var);
>>> +
>>> + if ((var->attr & mask) != mask) {
>>> + var = (void *)((uintptr_t)var + len);
>>> + continue;
>>> + }
>>> +
>>> + hdr.length += len;
>>> +
>>> + if (buf && hdr.length <= *lenp) {
>>> + efi_memcpy_runtime(var_to, var, len);
>>> + var_to = (void *)var_to + len;
>>> + }
>>> + var = (void *)var + len;
>>> + }
>>> +
>>> + if (!buf && hdr.length <= *lenp) {
>>> + *lenp = hdr.length;
>>> + return EFI_INVALID_PARAMETER;
>>> + }
>>> +
>>> + if (!buf || hdr.length > *lenp) {
>>> + *lenp = hdr.length;
>>> + return EFI_BUFFER_TOO_SMALL;
>>> + }
>>> + hdr.crc32 = crc32(0, (u8 *)buf->var,
>>> + hdr.length - sizeof(struct efi_var_file));
>>> +
>>> + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
>>> + *lenp = hdr.length;
>>> +
>>> + return EFI_SUCCESS;
>>> +}
>>> +
>>> efi_status_t __efi_runtime
>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>> - u64 *timep)
>>> + u64 *timep, u32 mask)
>>> {
>>> efi_uintn_t old_size;
>>> struct efi_var_entry *var;
>>> @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>> if (!var)
>>> return EFI_NOT_FOUND;
>>>
>>> + /*
>>> + * This function is used at runtime to dump EFI variables.
>>> + * The memory backend we keep around has BS-only variables as
>>> + * well. At runtime we filter them here
>>> + */
>>> + if (mask && !((var->attr & mask) == mask))
>>> + return EFI_NOT_FOUND;
>>> +
>>> if (attributes)
>>> *attributes = var->attr;
>>> if (timep)
>>> *timep = var->time;
>>>
>>> + if (!u16_strcmp(variable_name, u"VarToFile"))
>>> + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
>>> +
>>> old_size = *data_size;
>>> *data_size = var->length;
>>> if (old_size < var->length)
>>> @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>
>>> efi_status_t __efi_runtime
>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>> - u16 *variable_name, efi_guid_t *vendor)
>>> + u16 *variable_name, efi_guid_t *vendor,
>>> + u32 mask)
>>> {
>>> struct efi_var_entry *var;
>>> efi_uintn_t len, old_size;
>>> @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>> if (!variable_name_size || !variable_name || !vendor)
>>> return EFI_INVALID_PARAMETER;
>>>
>>> +skip:
>>> len = *variable_name_size >> 1;
>>> if (u16_strnlen(variable_name, len) == len)
>>> return EFI_INVALID_PARAMETER;
>>> @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>> efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
>>> efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
>>>
>>> + if (mask && !((var->attr & mask) == mask)) {
>>> + *variable_name_size = old_size;
>>> + goto skip;
>>> + }
>>> +
>>> return EFI_SUCCESS;
>>> }
>>>
>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>>> index 47bb79920575..0cbed53d1dbf 100644
>>> --- a/lib/efi_loader/efi_variable.c
>>> +++ b/lib/efi_loader/efi_variable.c
>>> @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>> u64 *timep)
>>> {
>>> - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
>>> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
>>> + data, timep, 0);
>>> }
>>>
>>> efi_status_t __efi_runtime
>>> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
>>> u16 *variable_name, efi_guid_t *vendor)
>>> {
>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
>>> + vendor, 0);
>>> }
>>>
>>> /**
>>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
>>> index dde135fd9f81..4f1aa298da13 100644
>>> --- a/lib/efi_loader/efi_variable_tee.c
>>> +++ b/lib/efi_loader/efi_variable_tee.c
>>> @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
>>> log_err("Unable to notify the MM partition for ExitBootServices\n");
>>> free(comm_buf);
>>>
>>> - /*
>>> - * Populate the list for runtime variables.
>>> - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
>>> - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
>>> - */
>>> ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
>>> if (ret != EFI_SUCCESS)
>>> log_err("Can't populate EFI variables. No runtime variables will be available\n");
>>> --
>>> 2.40.1
>>>
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 15:42 ` Heinrich Schuchardt
@ 2024-04-18 15:59 ` Ilias Apalodimas
2024-04-18 16:14 ` Heinrich Schuchardt
0 siblings, 1 reply; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 15:59 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: xypron.glpk, kettenis, caleb.connolly, sumit.garg, quic_llindhol,
ardb, pbrobinson, pjones, trini, kojima.masahisa,
takahiro.akashi, raymond.mao, sjg, matthias.schiffer, j,
abdellatif.elkhlifi, sughosh.ganu, richard.henderson, CFSworks,
alpernebiyasak, o451686892, u-boot, Mark Kettenis
On Thu, 18 Apr 2024 at 18:42, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 18.04.24 17:36, Ilias Apalodimas wrote:
> > Hi Mark,
> >
> > On Thu, 18 Apr 2024 at 18:15, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>
> >>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> Date: Thu, 18 Apr 2024 15:54:52 +0300
> >>
> >> Hi Illias,
> >>
> >>>
> >>> Previous patches enabled SetVariableRT using a RAM backend.
> >>> Although EBBR [0] defines a variable format we can teach userspace tools
> >>> and write the altered variables, it's better if we skip the ABI
> >>> requirements completely.
> >>>
> >>> So let's add a new variable, in its own namespace called "VarToFile"
> >>> which contains a binary dump of the updated RT, BS and, NV variables
> >>> and will be updated when GetVariable is called.
> >>>
> >>> Some adjustments are needed to do that.
> >>> Currently we discard BS-only variables in EBS(). We need to preserve
> >>> those on the RAM backend that exposes the variables. Since BS-only
> >>> variables can't appear at runtime we need to move the memory masking
> >>> checks from efi_var_collect() to efi_get_next_variable_name_mem()/
> >>> efi_get_variable_mem() and do the filtering at runtime.
> >>>
> >>> We also need an efi_var_collect() variant available at runtime, in order
> >>> to construct the "VarToFile" buffer on the fly.
> >>>
> >>> All users and applications (for linux) have to do when updating a variable
> >>> is dd that variable in the file described by "RTStorageVolatile".
> >>
> >> I simehow missed your reply to the issue I raised with picking the
> >> right ESP to write variables back to.
> >
> > No worries, I did send v3 quickly myself...
> >
> >> I'm not convinced that the ESP
> >> that was used to install Linux on is necessarily th right one. In
> >> particular, consider a system with multiple disks, say eMMC and an
> >> NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
> >> that disk to store the EFI variables.
> >
> > And who creates ESP on the eMMC? I assume you have one OS installed in
> > the eMMC and another one in the nvme?
>
> E.g. you have a naked NVMe. At first boot there will be an ESP on the
> installation medium. The installer may create an ESP on the NVMe drive
> and put GRUB there.
>
> Later the user will typically remove the installation module if U-Boot
> is on SPI flash.
>
> U-Boot has no chance to know this beforehand.
>
> Best regards
Ok thanks, Heinrich, as you mentioned offline that's what the Ubuntu
installer does.
In this case again, neither the DP nor the relative path directly
solves the problem, since that second ESP doesn't exist to begin with.
Can we get some more info on the installer? When it tries to write
Boot0000 which ESP is mounted? The 'installer' one or the newly
created one in the NVME? Because if it's the latter things should
work.
Thanks
/Ilias
>
> Heinrich
>
>
> >
> >> Now I install Linux on the NVMe
> >> disk, which creates an ESP to store its bootloader. With your
> >> proposed changes, Linux will probably end up writing the variables to
> >> the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
> >> variables from eMMC and you've lost any changes...
> >
> > I understand the problem, but my concern is that using a DP just
> > delegates the problem -- it doesn't solve it.
> >
> > To use the 'correct' partition, U-Boot has to *decide* which ESP is
> > going to be used and inject it in RTVolatileStorage.
> > But if U-Boot decides about the 'correct' ESP the relative path would
> > work as well. So what's needed here is a mechanism to correlate the
> > booted OS with the ESP it will use and force the variable loading from
> > that file. Am I missing anything?
> >
> > /Ilias
> >>
> >>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> >>> in little-endian format. So, storing variables works like this:
> >>>
> >>> $~ efibootmgr -n 0001
> >>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
> >>>
> >>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >>>
> >>> Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> >>> Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
> >>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>> include/efi_variable.h | 16 +++-
> >>> lib/charset.c | 2 +-
> >>> lib/efi_loader/efi_runtime.c | 25 +++++
> >>> lib/efi_loader/efi_var_common.c | 6 +-
> >>> lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
> >>> lib/efi_loader/efi_variable.c | 6 +-
> >>> lib/efi_loader/efi_variable_tee.c | 5 -
> >>> 7 files changed, 146 insertions(+), 65 deletions(-)
> >>>
> >>> diff --git a/include/efi_variable.h b/include/efi_variable.h
> >>> index 42a2b7c52bef..223bb9a4a5bd 100644
> >>> --- a/include/efi_variable.h
> >>> +++ b/include/efi_variable.h
> >>> @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
> >>> *
> >>> * @variable_name_size: size of variable_name buffer in bytes
> >>> * @variable_name: name of uefi variable's name in u16
> >>> + * @mask: bitmask with required attributes of variables to be collected.
> >>> + * variables are only collected if all of the required
> >>> + * attributes match. Use 0 to skip matching
> >>> * @vendor: vendor's guid
> >>> *
> >>> * Return: status code
> >>> */
> >>> efi_status_t __efi_runtime
> >>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> >>> - efi_guid_t *vendor);
> >>> + efi_guid_t *vendor, u32 mask);
> >>> /**
> >>> * efi_get_variable_mem() - Runtime common code across efi variable
> >>> * implementations for GetVariable() from
> >>> @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
> >>> * @data_size: size of the buffer to which the variable value is copied
> >>> * @data: buffer to which the variable value is copied
> >>> * @timep: authentication time (seconds since start of epoch)
> >>> + * @mask: bitmask with required attributes of variables to be collected.
> >>> + * variables are only collected if all of the required
> >>> + * attributes match. Use 0 to skip matching
> >>> * Return: status code
> >>> */
> >>> efi_status_t __efi_runtime
> >>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>> - u64 *timep);
> >>> + u64 *timep, u32 mask);
> >>>
> >>> /**
> >>> * efi_get_variable_runtime() - runtime implementation of GetVariable()
> >>> @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>> */
> >>> void efi_var_buf_update(struct efi_var_file *var_buf);
> >>>
> >>> +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
> >>> + efi_uintn_t *lenp,
> >>> + u32 check_attr_mask);
> >>> +
> >>> +u32 efi_var_entry_len(struct efi_var_entry *var);
> >>> +
> >>> #endif
> >>> diff --git a/lib/charset.c b/lib/charset.c
> >>> index df4f04074852..182c92a50c48 100644
> >>> --- a/lib/charset.c
> >>> +++ b/lib/charset.c
> >>> @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
> >>> * > 0 if the first different u16 in s1 is greater than the
> >>> * corresponding u16 in s2
> >>> */
> >>> -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> >>> +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> >>> {
> >>> int ret = 0;
> >>>
> >>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >>> index c8f7a88ba8db..73831c527e00 100644
> >>> --- a/lib/efi_loader/efi_runtime.c
> >>> +++ b/lib/efi_loader/efi_runtime.c
> >>> @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
> >>> EFI_RT_SUPPORTED_CONVERT_POINTER;
> >>>
> >>> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> >>> + u8 s = 0;
> >>> +
> >>> ret = efi_set_variable_int(u"RTStorageVolatile",
> >>> &efi_guid_efi_rt_var_file,
> >>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
> >>> log_err("Failed to set RTStorageVolatile\n");
> >>> return ret;
> >>> }
> >>> + /*
> >>> + * This variable needs to be visible so users can read it,
> >>> + * but the real contents are going to be filled during
> >>> + * GetVariable
> >>> + */
> >>> + ret = efi_set_variable_int(u"VarToFile",
> >>> + &efi_guid_efi_rt_var_file,
> >>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS |
> >>> + EFI_VARIABLE_READ_ONLY,
> >>> + sizeof(s),
> >>> + &s, false);
> >>> + if (ret != EFI_SUCCESS) {
> >>> + log_err("Failed to set VarToFile\n");
> >>> + efi_set_variable_int(u"RTStorageVolatile",
> >>> + &efi_guid_efi_rt_var_file,
> >>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS |
> >>> + EFI_VARIABLE_READ_ONLY,
> >>> + 0, NULL, false);
> >>> +
> >>> + return ret;
> >>> + }
> >>> rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
> >>> }
> >>>
> >>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> >>> index aa8feffd3ec1..7862f2d6ce8a 100644
> >>> --- a/lib/efi_loader/efi_var_common.c
> >>> +++ b/lib/efi_loader/efi_var_common.c
> >>> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> >>> {
> >>> efi_status_t ret;
> >>>
> >>> - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> >>> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
> >>> + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
> >>>
> >>> /* Remove EFI_VARIABLE_READ_ONLY flag */
> >>> if (attributes)
> >>> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
> >>> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>> u16 *variable_name, efi_guid_t *guid)
> >>> {
> >>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> >>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> >>> + guid, EFI_VARIABLE_RUNTIME_ACCESS);
> >>> }
> >>>
> >>> /**
> >>> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> >>> index 6c21cec5d457..940ab6638823 100644
> >>> --- a/lib/efi_loader/efi_var_mem.c
> >>> +++ b/lib/efi_loader/efi_var_mem.c
> >>> @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
> >>> return match;
> >>> }
> >>>
> >>> +/**
> >>> + * efi_var_entry_len() - Get the entry len including headers & name
> >>> + *
> >>> + * @var: pointer to variable start
> >>> + *
> >>> + * Return: 8-byte aligned variable entry length
> >>> + */
> >>> +
> >>> +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
> >>> +{
> >>> + if (!var)
> >>> + return 0;
> >>> +
> >>> + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
> >>> + var->length + sizeof(*var), 8);
> >>> +}
> >>> +
> >>> struct efi_var_entry __efi_runtime
> >>> *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
> >>> struct efi_var_entry **next)
> >>> @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
> >>> sizeof(struct efi_var_entry);
> >>> }
> >>>
> >>> -/**
> >>> - * efi_var_mem_bs_del() - delete boot service only variables
> >>> - */
> >>> -static void efi_var_mem_bs_del(void)
> >>> -{
> >>> - struct efi_var_entry *var = efi_var_buf->var;
> >>> -
> >>> - for (;;) {
> >>> - struct efi_var_entry *last;
> >>> -
> >>> - last = (struct efi_var_entry *)
> >>> - ((uintptr_t)efi_var_buf + efi_var_buf->length);
> >>> - if (var >= last)
> >>> - break;
> >>> - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> >>> - u16 *data;
> >>> -
> >>> - /* skip variable */
> >>> - for (data = var->name; *data; ++data)
> >>> - ;
> >>> - ++data;
> >>> - var = (struct efi_var_entry *)
> >>> - ALIGN((uintptr_t)data + var->length, 8);
> >>> - } else {
> >>> - /* delete variable */
> >>> - efi_var_mem_del(var);
> >>> - }
> >>> - }
> >>> -}
> >>> -
> >>> -/**
> >>> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> >>> - *
> >>> - * @event: callback event
> >>> - * @context: callback context
> >>> - */
> >>> -static void EFIAPI
> >>> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> >>> -{
> >>> - EFI_ENTRY("%p, %p", event, context);
> >>> -
> >>> - /* Delete boot service only variables */
> >>> - efi_var_mem_bs_del();
> >>> -
> >>> - EFI_EXIT(EFI_SUCCESS);
> >>> -}
> >>> -
> >>> /**
> >>> * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
> >>> *
> >>> @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
> >>> efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> >>> efi_var_buf->length = (uintptr_t)efi_var_buf->var -
> >>> (uintptr_t)efi_var_buf;
> >>> - /* crc32 for 0 bytes = 0 */
> >>>
> >>> - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> >>> - efi_var_mem_notify_exit_boot_services, NULL,
> >>> - NULL, &event);
> >>> if (ret != EFI_SUCCESS)
> >>> return ret;
> >>> ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
> >>> @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
> >>> return ret;
> >>> }
> >>>
> >>> +/**
> >>> + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
> >>> + * efi_var_buf
> >>> + *
> >>> + * @buf: buffer containing variable collection
> >>> + * @lenp: buffer length
> >>> + * @mask: mask of matched attributes
> >>> + *
> >>> + * Return: Status code
> >>> + */
> >>> +efi_status_t __efi_runtime
> >>> +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
> >>> +{
> >>> + static struct efi_var_file __efi_runtime_data hdr = {
> >>> + .magic = EFI_VAR_FILE_MAGIC,
> >>> + };
> >>> + struct efi_var_entry *last, *var, *var_to;
> >>> +
> >>> + hdr.length = sizeof(struct efi_var_file);
> >>> +
> >>> + var = efi_var_buf->var;
> >>> + last = (struct efi_var_entry *)
> >>> + ((uintptr_t)efi_var_buf + efi_var_buf->length);
> >>> + if (buf)
> >>> + var_to = buf->var;
> >>> +
> >>> + while (var < last) {
> >>> + u32 len = efi_var_entry_len(var);
> >>> +
> >>> + if ((var->attr & mask) != mask) {
> >>> + var = (void *)((uintptr_t)var + len);
> >>> + continue;
> >>> + }
> >>> +
> >>> + hdr.length += len;
> >>> +
> >>> + if (buf && hdr.length <= *lenp) {
> >>> + efi_memcpy_runtime(var_to, var, len);
> >>> + var_to = (void *)var_to + len;
> >>> + }
> >>> + var = (void *)var + len;
> >>> + }
> >>> +
> >>> + if (!buf && hdr.length <= *lenp) {
> >>> + *lenp = hdr.length;
> >>> + return EFI_INVALID_PARAMETER;
> >>> + }
> >>> +
> >>> + if (!buf || hdr.length > *lenp) {
> >>> + *lenp = hdr.length;
> >>> + return EFI_BUFFER_TOO_SMALL;
> >>> + }
> >>> + hdr.crc32 = crc32(0, (u8 *)buf->var,
> >>> + hdr.length - sizeof(struct efi_var_file));
> >>> +
> >>> + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
> >>> + *lenp = hdr.length;
> >>> +
> >>> + return EFI_SUCCESS;
> >>> +}
> >>> +
> >>> efi_status_t __efi_runtime
> >>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>> - u64 *timep)
> >>> + u64 *timep, u32 mask)
> >>> {
> >>> efi_uintn_t old_size;
> >>> struct efi_var_entry *var;
> >>> @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>> if (!var)
> >>> return EFI_NOT_FOUND;
> >>>
> >>> + /*
> >>> + * This function is used at runtime to dump EFI variables.
> >>> + * The memory backend we keep around has BS-only variables as
> >>> + * well. At runtime we filter them here
> >>> + */
> >>> + if (mask && !((var->attr & mask) == mask))
> >>> + return EFI_NOT_FOUND;
> >>> +
> >>> if (attributes)
> >>> *attributes = var->attr;
> >>> if (timep)
> >>> *timep = var->time;
> >>>
> >>> + if (!u16_strcmp(variable_name, u"VarToFile"))
> >>> + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
> >>> +
> >>> old_size = *data_size;
> >>> *data_size = var->length;
> >>> if (old_size < var->length)
> >>> @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>
> >>> efi_status_t __efi_runtime
> >>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>> - u16 *variable_name, efi_guid_t *vendor)
> >>> + u16 *variable_name, efi_guid_t *vendor,
> >>> + u32 mask)
> >>> {
> >>> struct efi_var_entry *var;
> >>> efi_uintn_t len, old_size;
> >>> @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>> if (!variable_name_size || !variable_name || !vendor)
> >>> return EFI_INVALID_PARAMETER;
> >>>
> >>> +skip:
> >>> len = *variable_name_size >> 1;
> >>> if (u16_strnlen(variable_name, len) == len)
> >>> return EFI_INVALID_PARAMETER;
> >>> @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>> efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> >>> efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
> >>>
> >>> + if (mask && !((var->attr & mask) == mask)) {
> >>> + *variable_name_size = old_size;
> >>> + goto skip;
> >>> + }
> >>> +
> >>> return EFI_SUCCESS;
> >>> }
> >>>
> >>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >>> index 47bb79920575..0cbed53d1dbf 100644
> >>> --- a/lib/efi_loader/efi_variable.c
> >>> +++ b/lib/efi_loader/efi_variable.c
> >>> @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
> >>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>> u64 *timep)
> >>> {
> >>> - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> >>> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
> >>> + data, timep, 0);
> >>> }
> >>>
> >>> efi_status_t __efi_runtime
> >>> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> >>> u16 *variable_name, efi_guid_t *vendor)
> >>> {
> >>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> >>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> >>> + vendor, 0);
> >>> }
> >>>
> >>> /**
> >>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> >>> index dde135fd9f81..4f1aa298da13 100644
> >>> --- a/lib/efi_loader/efi_variable_tee.c
> >>> +++ b/lib/efi_loader/efi_variable_tee.c
> >>> @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
> >>> log_err("Unable to notify the MM partition for ExitBootServices\n");
> >>> free(comm_buf);
> >>>
> >>> - /*
> >>> - * Populate the list for runtime variables.
> >>> - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
> >>> - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
> >>> - */
> >>> ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
> >>> if (ret != EFI_SUCCESS)
> >>> log_err("Can't populate EFI variables. No runtime variables will be available\n");
> >>> --
> >>> 2.40.1
> >>>
> >>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 15:59 ` Ilias Apalodimas
@ 2024-04-18 16:14 ` Heinrich Schuchardt
2024-04-18 17:03 ` Ilias Apalodimas
0 siblings, 1 reply; 15+ messages in thread
From: Heinrich Schuchardt @ 2024-04-18 16:14 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: kettenis, caleb.connolly, sumit.garg, quic_llindhol, ardb,
pbrobinson, pjones, trini, kojima.masahisa, takahiro.akashi,
raymond.mao, sjg, matthias.schiffer, j, abdellatif.elkhlifi,
sughosh.ganu, richard.henderson, CFSworks, alpernebiyasak,
o451686892, u-boot, Mark Kettenis
On 18.04.24 17:59, Ilias Apalodimas wrote:
> On Thu, 18 Apr 2024 at 18:42, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 18.04.24 17:36, Ilias Apalodimas wrote:
>>> Hi Mark,
>>>
>>> On Thu, 18 Apr 2024 at 18:15, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
>>>>
>>>>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> Date: Thu, 18 Apr 2024 15:54:52 +0300
>>>>
>>>> Hi Illias,
>>>>
>>>>>
>>>>> Previous patches enabled SetVariableRT using a RAM backend.
>>>>> Although EBBR [0] defines a variable format we can teach userspace tools
>>>>> and write the altered variables, it's better if we skip the ABI
>>>>> requirements completely.
>>>>>
>>>>> So let's add a new variable, in its own namespace called "VarToFile"
>>>>> which contains a binary dump of the updated RT, BS and, NV variables
>>>>> and will be updated when GetVariable is called.
>>>>>
>>>>> Some adjustments are needed to do that.
>>>>> Currently we discard BS-only variables in EBS(). We need to preserve
>>>>> those on the RAM backend that exposes the variables. Since BS-only
>>>>> variables can't appear at runtime we need to move the memory masking
>>>>> checks from efi_var_collect() to efi_get_next_variable_name_mem()/
>>>>> efi_get_variable_mem() and do the filtering at runtime.
>>>>>
>>>>> We also need an efi_var_collect() variant available at runtime, in order
>>>>> to construct the "VarToFile" buffer on the fly.
>>>>>
>>>>> All users and applications (for linux) have to do when updating a variable
>>>>> is dd that variable in the file described by "RTStorageVolatile".
>>>>
>>>> I simehow missed your reply to the issue I raised with picking the
>>>> right ESP to write variables back to.
>>>
>>> No worries, I did send v3 quickly myself...
>>>
>>>> I'm not convinced that the ESP
>>>> that was used to install Linux on is necessarily th right one. In
>>>> particular, consider a system with multiple disks, say eMMC and an
>>>> NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
>>>> that disk to store the EFI variables.
>>>
>>> And who creates ESP on the eMMC? I assume you have one OS installed in
>>> the eMMC and another one in the nvme?
>>
>> E.g. you have a naked NVMe. At first boot there will be an ESP on the
>> installation medium. The installer may create an ESP on the NVMe drive
>> and put GRUB there.
>>
>> Later the user will typically remove the installation module if U-Boot
>> is on SPI flash.
>>
>> U-Boot has no chance to know this beforehand.
>>
>> Best regards
>
> Ok thanks, Heinrich, as you mentioned offline that's what the Ubuntu
> installer does.
> In this case again, neither the DP nor the relative path directly
> solves the problem, since that second ESP doesn't exist to begin with.
>
> Can we get some more info on the installer? When it tries to write
> Boot0000 which ESP is mounted? The 'installer' one or the newly
> created one in the NVME? Because if it's the latter things should
> work.
The ESP on the NVMe is mounted at /boot/efi. This is where GRUB is
installed as (/boot/efi)/EFI/ubuntu/grubriscv64.efi.
Best regards
Heinrich
>>
>>>
>>>> Now I install Linux on the NVMe
>>>> disk, which creates an ESP to store its bootloader. With your
>>>> proposed changes, Linux will probably end up writing the variables to
>>>> the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
>>>> variables from eMMC and you've lost any changes...
>>>
>>> I understand the problem, but my concern is that using a DP just
>>> delegates the problem -- it doesn't solve it.
>>>
>>> To use the 'correct' partition, U-Boot has to *decide* which ESP is
>>> going to be used and inject it in RTVolatileStorage.
>>> But if U-Boot decides about the 'correct' ESP the relative path would
>>> work as well. So what's needed here is a mechanism to correlate the
>>> booted OS with the ESP it will use and force the variable loading from
>>> that file. Am I missing anything?
>>>
>>> /Ilias
>>>>
>>>>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
>>>>> in little-endian format. So, storing variables works like this:
>>>>>
>>>>> $~ efibootmgr -n 0001
>>>>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
>>>>>
>>>>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
>>>>>
>>>>> Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
>>>>> Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
>>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>>> ---
>>>>> include/efi_variable.h | 16 +++-
>>>>> lib/charset.c | 2 +-
>>>>> lib/efi_loader/efi_runtime.c | 25 +++++
>>>>> lib/efi_loader/efi_var_common.c | 6 +-
>>>>> lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
>>>>> lib/efi_loader/efi_variable.c | 6 +-
>>>>> lib/efi_loader/efi_variable_tee.c | 5 -
>>>>> 7 files changed, 146 insertions(+), 65 deletions(-)
>>>>>
>>>>> diff --git a/include/efi_variable.h b/include/efi_variable.h
>>>>> index 42a2b7c52bef..223bb9a4a5bd 100644
>>>>> --- a/include/efi_variable.h
>>>>> +++ b/include/efi_variable.h
>>>>> @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
>>>>> *
>>>>> * @variable_name_size: size of variable_name buffer in bytes
>>>>> * @variable_name: name of uefi variable's name in u16
>>>>> + * @mask: bitmask with required attributes of variables to be collected.
>>>>> + * variables are only collected if all of the required
>>>>> + * attributes match. Use 0 to skip matching
>>>>> * @vendor: vendor's guid
>>>>> *
>>>>> * Return: status code
>>>>> */
>>>>> efi_status_t __efi_runtime
>>>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
>>>>> - efi_guid_t *vendor);
>>>>> + efi_guid_t *vendor, u32 mask);
>>>>> /**
>>>>> * efi_get_variable_mem() - Runtime common code across efi variable
>>>>> * implementations for GetVariable() from
>>>>> @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
>>>>> * @data_size: size of the buffer to which the variable value is copied
>>>>> * @data: buffer to which the variable value is copied
>>>>> * @timep: authentication time (seconds since start of epoch)
>>>>> + * @mask: bitmask with required attributes of variables to be collected.
>>>>> + * variables are only collected if all of the required
>>>>> + * attributes match. Use 0 to skip matching
>>>>> * Return: status code
>>>>> */
>>>>> efi_status_t __efi_runtime
>>>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>>>> - u64 *timep);
>>>>> + u64 *timep, u32 mask);
>>>>>
>>>>> /**
>>>>> * efi_get_variable_runtime() - runtime implementation of GetVariable()
>>>>> @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>>>>> */
>>>>> void efi_var_buf_update(struct efi_var_file *var_buf);
>>>>>
>>>>> +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
>>>>> + efi_uintn_t *lenp,
>>>>> + u32 check_attr_mask);
>>>>> +
>>>>> +u32 efi_var_entry_len(struct efi_var_entry *var);
>>>>> +
>>>>> #endif
>>>>> diff --git a/lib/charset.c b/lib/charset.c
>>>>> index df4f04074852..182c92a50c48 100644
>>>>> --- a/lib/charset.c
>>>>> +++ b/lib/charset.c
>>>>> @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
>>>>> * > 0 if the first different u16 in s1 is greater than the
>>>>> * corresponding u16 in s2
>>>>> */
>>>>> -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
>>>>> +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
>>>>> {
>>>>> int ret = 0;
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
>>>>> index c8f7a88ba8db..73831c527e00 100644
>>>>> --- a/lib/efi_loader/efi_runtime.c
>>>>> +++ b/lib/efi_loader/efi_runtime.c
>>>>> @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
>>>>> EFI_RT_SUPPORTED_CONVERT_POINTER;
>>>>>
>>>>> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
>>>>> + u8 s = 0;
>>>>> +
>>>>> ret = efi_set_variable_int(u"RTStorageVolatile",
>>>>> &efi_guid_efi_rt_var_file,
>>>>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>> @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
>>>>> log_err("Failed to set RTStorageVolatile\n");
>>>>> return ret;
>>>>> }
>>>>> + /*
>>>>> + * This variable needs to be visible so users can read it,
>>>>> + * but the real contents are going to be filled during
>>>>> + * GetVariable
>>>>> + */
>>>>> + ret = efi_set_variable_int(u"VarToFile",
>>>>> + &efi_guid_efi_rt_var_file,
>>>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>> + EFI_VARIABLE_RUNTIME_ACCESS |
>>>>> + EFI_VARIABLE_READ_ONLY,
>>>>> + sizeof(s),
>>>>> + &s, false);
>>>>> + if (ret != EFI_SUCCESS) {
>>>>> + log_err("Failed to set VarToFile\n");
>>>>> + efi_set_variable_int(u"RTStorageVolatile",
>>>>> + &efi_guid_efi_rt_var_file,
>>>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>> + EFI_VARIABLE_RUNTIME_ACCESS |
>>>>> + EFI_VARIABLE_READ_ONLY,
>>>>> + 0, NULL, false);
>>>>> +
>>>>> + return ret;
>>>>> + }
>>>>> rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
>>>>> }
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
>>>>> index aa8feffd3ec1..7862f2d6ce8a 100644
>>>>> --- a/lib/efi_loader/efi_var_common.c
>>>>> +++ b/lib/efi_loader/efi_var_common.c
>>>>> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
>>>>> {
>>>>> efi_status_t ret;
>>>>>
>>>>> - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
>>>>> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
>>>>> + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
>>>>>
>>>>> /* Remove EFI_VARIABLE_READ_ONLY flag */
>>>>> if (attributes)
>>>>> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
>>>>> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
>>>>> u16 *variable_name, efi_guid_t *guid)
>>>>> {
>>>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
>>>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
>>>>> + guid, EFI_VARIABLE_RUNTIME_ACCESS);
>>>>> }
>>>>>
>>>>> /**
>>>>> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
>>>>> index 6c21cec5d457..940ab6638823 100644
>>>>> --- a/lib/efi_loader/efi_var_mem.c
>>>>> +++ b/lib/efi_loader/efi_var_mem.c
>>>>> @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
>>>>> return match;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * efi_var_entry_len() - Get the entry len including headers & name
>>>>> + *
>>>>> + * @var: pointer to variable start
>>>>> + *
>>>>> + * Return: 8-byte aligned variable entry length
>>>>> + */
>>>>> +
>>>>> +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
>>>>> +{
>>>>> + if (!var)
>>>>> + return 0;
>>>>> +
>>>>> + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
>>>>> + var->length + sizeof(*var), 8);
>>>>> +}
>>>>> +
>>>>> struct efi_var_entry __efi_runtime
>>>>> *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
>>>>> struct efi_var_entry **next)
>>>>> @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
>>>>> sizeof(struct efi_var_entry);
>>>>> }
>>>>>
>>>>> -/**
>>>>> - * efi_var_mem_bs_del() - delete boot service only variables
>>>>> - */
>>>>> -static void efi_var_mem_bs_del(void)
>>>>> -{
>>>>> - struct efi_var_entry *var = efi_var_buf->var;
>>>>> -
>>>>> - for (;;) {
>>>>> - struct efi_var_entry *last;
>>>>> -
>>>>> - last = (struct efi_var_entry *)
>>>>> - ((uintptr_t)efi_var_buf + efi_var_buf->length);
>>>>> - if (var >= last)
>>>>> - break;
>>>>> - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
>>>>> - u16 *data;
>>>>> -
>>>>> - /* skip variable */
>>>>> - for (data = var->name; *data; ++data)
>>>>> - ;
>>>>> - ++data;
>>>>> - var = (struct efi_var_entry *)
>>>>> - ALIGN((uintptr_t)data + var->length, 8);
>>>>> - } else {
>>>>> - /* delete variable */
>>>>> - efi_var_mem_del(var);
>>>>> - }
>>>>> - }
>>>>> -}
>>>>> -
>>>>> -/**
>>>>> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
>>>>> - *
>>>>> - * @event: callback event
>>>>> - * @context: callback context
>>>>> - */
>>>>> -static void EFIAPI
>>>>> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
>>>>> -{
>>>>> - EFI_ENTRY("%p, %p", event, context);
>>>>> -
>>>>> - /* Delete boot service only variables */
>>>>> - efi_var_mem_bs_del();
>>>>> -
>>>>> - EFI_EXIT(EFI_SUCCESS);
>>>>> -}
>>>>> -
>>>>> /**
>>>>> * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
>>>>> *
>>>>> @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
>>>>> efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
>>>>> efi_var_buf->length = (uintptr_t)efi_var_buf->var -
>>>>> (uintptr_t)efi_var_buf;
>>>>> - /* crc32 for 0 bytes = 0 */
>>>>>
>>>>> - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
>>>>> - efi_var_mem_notify_exit_boot_services, NULL,
>>>>> - NULL, &event);
>>>>> if (ret != EFI_SUCCESS)
>>>>> return ret;
>>>>> ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
>>>>> @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
>>>>> return ret;
>>>>> }
>>>>>
>>>>> +/**
>>>>> + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
>>>>> + * efi_var_buf
>>>>> + *
>>>>> + * @buf: buffer containing variable collection
>>>>> + * @lenp: buffer length
>>>>> + * @mask: mask of matched attributes
>>>>> + *
>>>>> + * Return: Status code
>>>>> + */
>>>>> +efi_status_t __efi_runtime
>>>>> +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
>>>>> +{
>>>>> + static struct efi_var_file __efi_runtime_data hdr = {
>>>>> + .magic = EFI_VAR_FILE_MAGIC,
>>>>> + };
>>>>> + struct efi_var_entry *last, *var, *var_to;
>>>>> +
>>>>> + hdr.length = sizeof(struct efi_var_file);
>>>>> +
>>>>> + var = efi_var_buf->var;
>>>>> + last = (struct efi_var_entry *)
>>>>> + ((uintptr_t)efi_var_buf + efi_var_buf->length);
>>>>> + if (buf)
>>>>> + var_to = buf->var;
>>>>> +
>>>>> + while (var < last) {
>>>>> + u32 len = efi_var_entry_len(var);
>>>>> +
>>>>> + if ((var->attr & mask) != mask) {
>>>>> + var = (void *)((uintptr_t)var + len);
>>>>> + continue;
>>>>> + }
>>>>> +
>>>>> + hdr.length += len;
>>>>> +
>>>>> + if (buf && hdr.length <= *lenp) {
>>>>> + efi_memcpy_runtime(var_to, var, len);
>>>>> + var_to = (void *)var_to + len;
>>>>> + }
>>>>> + var = (void *)var + len;
>>>>> + }
>>>>> +
>>>>> + if (!buf && hdr.length <= *lenp) {
>>>>> + *lenp = hdr.length;
>>>>> + return EFI_INVALID_PARAMETER;
>>>>> + }
>>>>> +
>>>>> + if (!buf || hdr.length > *lenp) {
>>>>> + *lenp = hdr.length;
>>>>> + return EFI_BUFFER_TOO_SMALL;
>>>>> + }
>>>>> + hdr.crc32 = crc32(0, (u8 *)buf->var,
>>>>> + hdr.length - sizeof(struct efi_var_file));
>>>>> +
>>>>> + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
>>>>> + *lenp = hdr.length;
>>>>> +
>>>>> + return EFI_SUCCESS;
>>>>> +}
>>>>> +
>>>>> efi_status_t __efi_runtime
>>>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>>>> - u64 *timep)
>>>>> + u64 *timep, u32 mask)
>>>>> {
>>>>> efi_uintn_t old_size;
>>>>> struct efi_var_entry *var;
>>>>> @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>>> if (!var)
>>>>> return EFI_NOT_FOUND;
>>>>>
>>>>> + /*
>>>>> + * This function is used at runtime to dump EFI variables.
>>>>> + * The memory backend we keep around has BS-only variables as
>>>>> + * well. At runtime we filter them here
>>>>> + */
>>>>> + if (mask && !((var->attr & mask) == mask))
>>>>> + return EFI_NOT_FOUND;
>>>>> +
>>>>> if (attributes)
>>>>> *attributes = var->attr;
>>>>> if (timep)
>>>>> *timep = var->time;
>>>>>
>>>>> + if (!u16_strcmp(variable_name, u"VarToFile"))
>>>>> + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
>>>>> +
>>>>> old_size = *data_size;
>>>>> *data_size = var->length;
>>>>> if (old_size < var->length)
>>>>> @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
>>>>>
>>>>> efi_status_t __efi_runtime
>>>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>>>> - u16 *variable_name, efi_guid_t *vendor)
>>>>> + u16 *variable_name, efi_guid_t *vendor,
>>>>> + u32 mask)
>>>>> {
>>>>> struct efi_var_entry *var;
>>>>> efi_uintn_t len, old_size;
>>>>> @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>>>> if (!variable_name_size || !variable_name || !vendor)
>>>>> return EFI_INVALID_PARAMETER;
>>>>>
>>>>> +skip:
>>>>> len = *variable_name_size >> 1;
>>>>> if (u16_strnlen(variable_name, len) == len)
>>>>> return EFI_INVALID_PARAMETER;
>>>>> @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
>>>>> efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
>>>>> efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
>>>>>
>>>>> + if (mask && !((var->attr & mask) == mask)) {
>>>>> + *variable_name_size = old_size;
>>>>> + goto skip;
>>>>> + }
>>>>> +
>>>>> return EFI_SUCCESS;
>>>>> }
>>>>>
>>>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
>>>>> index 47bb79920575..0cbed53d1dbf 100644
>>>>> --- a/lib/efi_loader/efi_variable.c
>>>>> +++ b/lib/efi_loader/efi_variable.c
>>>>> @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
>>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
>>>>> u64 *timep)
>>>>> {
>>>>> - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
>>>>> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
>>>>> + data, timep, 0);
>>>>> }
>>>>>
>>>>> efi_status_t __efi_runtime
>>>>> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
>>>>> u16 *variable_name, efi_guid_t *vendor)
>>>>> {
>>>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
>>>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
>>>>> + vendor, 0);
>>>>> }
>>>>>
>>>>> /**
>>>>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
>>>>> index dde135fd9f81..4f1aa298da13 100644
>>>>> --- a/lib/efi_loader/efi_variable_tee.c
>>>>> +++ b/lib/efi_loader/efi_variable_tee.c
>>>>> @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
>>>>> log_err("Unable to notify the MM partition for ExitBootServices\n");
>>>>> free(comm_buf);
>>>>>
>>>>> - /*
>>>>> - * Populate the list for runtime variables.
>>>>> - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
>>>>> - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
>>>>> - */
>>>>> ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
>>>>> if (ret != EFI_SUCCESS)
>>>>> log_err("Can't populate EFI variables. No runtime variables will be available\n");
>>>>> --
>>>>> 2.40.1
>>>>>
>>>>>
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents
2024-04-18 16:14 ` Heinrich Schuchardt
@ 2024-04-18 17:03 ` Ilias Apalodimas
0 siblings, 0 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-18 17:03 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: kettenis, caleb.connolly, sumit.garg, quic_llindhol, ardb,
pbrobinson, pjones, trini, kojima.masahisa, takahiro.akashi,
raymond.mao, sjg, matthias.schiffer, j, abdellatif.elkhlifi,
sughosh.ganu, richard.henderson, CFSworks, alpernebiyasak,
o451686892, u-boot, Mark Kettenis
On Thu, 18 Apr 2024 at 19:15, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 18.04.24 17:59, Ilias Apalodimas wrote:
> > On Thu, 18 Apr 2024 at 18:42, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 18.04.24 17:36, Ilias Apalodimas wrote:
> >>> Hi Mark,
> >>>
> >>> On Thu, 18 Apr 2024 at 18:15, Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> >>>>
> >>>>> From: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>> Date: Thu, 18 Apr 2024 15:54:52 +0300
> >>>>
> >>>> Hi Illias,
> >>>>
> >>>>>
> >>>>> Previous patches enabled SetVariableRT using a RAM backend.
> >>>>> Although EBBR [0] defines a variable format we can teach userspace tools
> >>>>> and write the altered variables, it's better if we skip the ABI
> >>>>> requirements completely.
> >>>>>
> >>>>> So let's add a new variable, in its own namespace called "VarToFile"
> >>>>> which contains a binary dump of the updated RT, BS and, NV variables
> >>>>> and will be updated when GetVariable is called.
> >>>>>
> >>>>> Some adjustments are needed to do that.
> >>>>> Currently we discard BS-only variables in EBS(). We need to preserve
> >>>>> those on the RAM backend that exposes the variables. Since BS-only
> >>>>> variables can't appear at runtime we need to move the memory masking
> >>>>> checks from efi_var_collect() to efi_get_next_variable_name_mem()/
> >>>>> efi_get_variable_mem() and do the filtering at runtime.
> >>>>>
> >>>>> We also need an efi_var_collect() variant available at runtime, in order
> >>>>> to construct the "VarToFile" buffer on the fly.
> >>>>>
> >>>>> All users and applications (for linux) have to do when updating a variable
> >>>>> is dd that variable in the file described by "RTStorageVolatile".
> >>>>
> >>>> I simehow missed your reply to the issue I raised with picking the
> >>>> right ESP to write variables back to.
> >>>
> >>> No worries, I did send v3 quickly myself...
> >>>
> >>>> I'm not convinced that the ESP
> >>>> that was used to install Linux on is necessarily th right one. In
> >>>> particular, consider a system with multiple disks, say eMMC and an
> >>>> NVMe disk. Suppose U-Boot is installed on eMMC and picks the ESP on
> >>>> that disk to store the EFI variables.
> >>>
> >>> And who creates ESP on the eMMC? I assume you have one OS installed in
> >>> the eMMC and another one in the nvme?
> >>
> >> E.g. you have a naked NVMe. At first boot there will be an ESP on the
> >> installation medium. The installer may create an ESP on the NVMe drive
> >> and put GRUB there.
> >>
> >> Later the user will typically remove the installation module if U-Boot
> >> is on SPI flash.
> >>
> >> U-Boot has no chance to know this beforehand.
> >>
> >> Best regards
> >
> > Ok thanks, Heinrich, as you mentioned offline that's what the Ubuntu
> > installer does.
> > In this case again, neither the DP nor the relative path directly
> > solves the problem, since that second ESP doesn't exist to begin with.
> >
> > Can we get some more info on the installer? When it tries to write
> > Boot0000 which ESP is mounted? The 'installer' one or the newly
> > created one in the NVME? Because if it's the latter things should
> > work.
>
> The ESP on the NVMe is mounted at /boot/efi. This is where GRUB is
> installed as (/boot/efi)/EFI/ubuntu/grubriscv64.efi.
Yes but if I understand you correctly the installer will work just
fine. Let me try and describe the process in case we are missing
something
- The installer starts from a USB and contains an ESP which is mounted
if I understand you correctly
- The installation creates a new ESP
- Once the installation is about to end, the new ESP is mounted and
grub/shim is copied over. At that point, it will also try to set
Boot0000 pointting to shim. But the installer *knows* which ESP it
mounted, so it can use that partition to write the updated
ubootefi.var
Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >>
> >>>
> >>>> Now I install Linux on the NVMe
> >>>> disk, which creates an ESP to store its bootloader. With your
> >>>> proposed changes, Linux will probably end up writing the variables to
> >>>> the ESP on the NVMe. Now you reboot and U-Boot still reads the EFI
> >>>> variables from eMMC and you've lost any changes...
> >>>
> >>> I understand the problem, but my concern is that using a DP just
> >>> delegates the problem -- it doesn't solve it.
> >>>
> >>> To use the 'correct' partition, U-Boot has to *decide* which ESP is
> >>> going to be used and inject it in RTVolatileStorage.
> >>> But if U-Boot decides about the 'correct' ESP the relative path would
> >>> work as well. So what's needed here is a mechanism to correlate the
> >>> booted OS with the ESP it will use and force the variable loading from
> >>> that file. Am I missing anything?
> >>>
> >>> /Ilias
> >>>>
> >>>>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> >>>>> in little-endian format. So, storing variables works like this:
> >>>>>
> >>>>> $~ efibootmgr -n 0001
> >>>>> $~ dd if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c of=/boot/efi/ubootefi.var skip=4 bs=1
> >>>>>
> >>>>> [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >>>>>
> >>>>> Suggested-by: Ard Biesheuvel <ardb@kernel.org> # dumping all variables to a variable
> >>>>> Co-developed-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com> # contributed on efi_var_collect_mem()
> >>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>>> ---
> >>>>> include/efi_variable.h | 16 +++-
> >>>>> lib/charset.c | 2 +-
> >>>>> lib/efi_loader/efi_runtime.c | 25 +++++
> >>>>> lib/efi_loader/efi_var_common.c | 6 +-
> >>>>> lib/efi_loader/efi_var_mem.c | 151 +++++++++++++++++++-----------
> >>>>> lib/efi_loader/efi_variable.c | 6 +-
> >>>>> lib/efi_loader/efi_variable_tee.c | 5 -
> >>>>> 7 files changed, 146 insertions(+), 65 deletions(-)
> >>>>>
> >>>>> diff --git a/include/efi_variable.h b/include/efi_variable.h
> >>>>> index 42a2b7c52bef..223bb9a4a5bd 100644
> >>>>> --- a/include/efi_variable.h
> >>>>> +++ b/include/efi_variable.h
> >>>>> @@ -271,13 +271,16 @@ const efi_guid_t *efi_auth_var_get_guid(const u16 *name);
> >>>>> *
> >>>>> * @variable_name_size: size of variable_name buffer in bytes
> >>>>> * @variable_name: name of uefi variable's name in u16
> >>>>> + * @mask: bitmask with required attributes of variables to be collected.
> >>>>> + * variables are only collected if all of the required
> >>>>> + * attributes match. Use 0 to skip matching
> >>>>> * @vendor: vendor's guid
> >>>>> *
> >>>>> * Return: status code
> >>>>> */
> >>>>> efi_status_t __efi_runtime
> >>>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_name,
> >>>>> - efi_guid_t *vendor);
> >>>>> + efi_guid_t *vendor, u32 mask);
> >>>>> /**
> >>>>> * efi_get_variable_mem() - Runtime common code across efi variable
> >>>>> * implementations for GetVariable() from
> >>>>> @@ -289,12 +292,15 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size, u16 *variable_na
> >>>>> * @data_size: size of the buffer to which the variable value is copied
> >>>>> * @data: buffer to which the variable value is copied
> >>>>> * @timep: authentication time (seconds since start of epoch)
> >>>>> + * @mask: bitmask with required attributes of variables to be collected.
> >>>>> + * variables are only collected if all of the required
> >>>>> + * attributes match. Use 0 to skip matching
> >>>>> * Return: status code
> >>>>> */
> >>>>> efi_status_t __efi_runtime
> >>>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>>>> - u64 *timep);
> >>>>> + u64 *timep, u32 mask);
> >>>>>
> >>>>> /**
> >>>>> * efi_get_variable_runtime() - runtime implementation of GetVariable()
> >>>>> @@ -334,4 +340,10 @@ efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>>>> */
> >>>>> void efi_var_buf_update(struct efi_var_file *var_buf);
> >>>>>
> >>>>> +efi_status_t __efi_runtime efi_var_collect_mem(struct efi_var_file *buf,
> >>>>> + efi_uintn_t *lenp,
> >>>>> + u32 check_attr_mask);
> >>>>> +
> >>>>> +u32 efi_var_entry_len(struct efi_var_entry *var);
> >>>>> +
> >>>>> #endif
> >>>>> diff --git a/lib/charset.c b/lib/charset.c
> >>>>> index df4f04074852..182c92a50c48 100644
> >>>>> --- a/lib/charset.c
> >>>>> +++ b/lib/charset.c
> >>>>> @@ -387,7 +387,7 @@ int u16_strcasecmp(const u16 *s1, const u16 *s2)
> >>>>> * > 0 if the first different u16 in s1 is greater than the
> >>>>> * corresponding u16 in s2
> >>>>> */
> >>>>> -int u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> >>>>> +int __efi_runtime u16_strncmp(const u16 *s1, const u16 *s2, size_t n)
> >>>>> {
> >>>>> int ret = 0;
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> >>>>> index c8f7a88ba8db..73831c527e00 100644
> >>>>> --- a/lib/efi_loader/efi_runtime.c
> >>>>> +++ b/lib/efi_loader/efi_runtime.c
> >>>>> @@ -130,6 +130,8 @@ efi_status_t efi_init_runtime_supported(void)
> >>>>> EFI_RT_SUPPORTED_CONVERT_POINTER;
> >>>>>
> >>>>> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> >>>>> + u8 s = 0;
> >>>>> +
> >>>>> ret = efi_set_variable_int(u"RTStorageVolatile",
> >>>>> &efi_guid_efi_rt_var_file,
> >>>>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>>>> @@ -141,6 +143,29 @@ efi_status_t efi_init_runtime_supported(void)
> >>>>> log_err("Failed to set RTStorageVolatile\n");
> >>>>> return ret;
> >>>>> }
> >>>>> + /*
> >>>>> + * This variable needs to be visible so users can read it,
> >>>>> + * but the real contents are going to be filled during
> >>>>> + * GetVariable
> >>>>> + */
> >>>>> + ret = efi_set_variable_int(u"VarToFile",
> >>>>> + &efi_guid_efi_rt_var_file,
> >>>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>>>> + EFI_VARIABLE_RUNTIME_ACCESS |
> >>>>> + EFI_VARIABLE_READ_ONLY,
> >>>>> + sizeof(s),
> >>>>> + &s, false);
> >>>>> + if (ret != EFI_SUCCESS) {
> >>>>> + log_err("Failed to set VarToFile\n");
> >>>>> + efi_set_variable_int(u"RTStorageVolatile",
> >>>>> + &efi_guid_efi_rt_var_file,
> >>>>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>>>> + EFI_VARIABLE_RUNTIME_ACCESS |
> >>>>> + EFI_VARIABLE_READ_ONLY,
> >>>>> + 0, NULL, false);
> >>>>> +
> >>>>> + return ret;
> >>>>> + }
> >>>>> rt_table->runtime_services_supported |= EFI_RT_SUPPORTED_SET_VARIABLE;
> >>>>> }
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> >>>>> index aa8feffd3ec1..7862f2d6ce8a 100644
> >>>>> --- a/lib/efi_loader/efi_var_common.c
> >>>>> +++ b/lib/efi_loader/efi_var_common.c
> >>>>> @@ -182,7 +182,8 @@ efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> >>>>> {
> >>>>> efi_status_t ret;
> >>>>>
> >>>>> - ret = efi_get_variable_mem(variable_name, guid, attributes, data_size, data, NULL);
> >>>>> + ret = efi_get_variable_mem(variable_name, guid, attributes, data_size,
> >>>>> + data, NULL, EFI_VARIABLE_RUNTIME_ACCESS);
> >>>>>
> >>>>> /* Remove EFI_VARIABLE_READ_ONLY flag */
> >>>>> if (attributes)
> >>>>> @@ -195,7 +196,8 @@ efi_status_t __efi_runtime EFIAPI
> >>>>> efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> >>>>> u16 *variable_name, efi_guid_t *guid)
> >>>>> {
> >>>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, guid);
> >>>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> >>>>> + guid, EFI_VARIABLE_RUNTIME_ACCESS);
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> diff --git a/lib/efi_loader/efi_var_mem.c b/lib/efi_loader/efi_var_mem.c
> >>>>> index 6c21cec5d457..940ab6638823 100644
> >>>>> --- a/lib/efi_loader/efi_var_mem.c
> >>>>> +++ b/lib/efi_loader/efi_var_mem.c
> >>>>> @@ -61,6 +61,23 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid,
> >>>>> return match;
> >>>>> }
> >>>>>
> >>>>> +/**
> >>>>> + * efi_var_entry_len() - Get the entry len including headers & name
> >>>>> + *
> >>>>> + * @var: pointer to variable start
> >>>>> + *
> >>>>> + * Return: 8-byte aligned variable entry length
> >>>>> + */
> >>>>> +
> >>>>> +u32 __efi_runtime efi_var_entry_len(struct efi_var_entry *var)
> >>>>> +{
> >>>>> + if (!var)
> >>>>> + return 0;
> >>>>> +
> >>>>> + return ALIGN((sizeof(u16) * (u16_strlen(var->name) + 1)) +
> >>>>> + var->length + sizeof(*var), 8);
> >>>>> +}
> >>>>> +
> >>>>> struct efi_var_entry __efi_runtime
> >>>>> *efi_var_mem_find(const efi_guid_t *guid, const u16 *name,
> >>>>> struct efi_var_entry **next)
> >>>>> @@ -184,53 +201,6 @@ u64 __efi_runtime efi_var_mem_free(void)
> >>>>> sizeof(struct efi_var_entry);
> >>>>> }
> >>>>>
> >>>>> -/**
> >>>>> - * efi_var_mem_bs_del() - delete boot service only variables
> >>>>> - */
> >>>>> -static void efi_var_mem_bs_del(void)
> >>>>> -{
> >>>>> - struct efi_var_entry *var = efi_var_buf->var;
> >>>>> -
> >>>>> - for (;;) {
> >>>>> - struct efi_var_entry *last;
> >>>>> -
> >>>>> - last = (struct efi_var_entry *)
> >>>>> - ((uintptr_t)efi_var_buf + efi_var_buf->length);
> >>>>> - if (var >= last)
> >>>>> - break;
> >>>>> - if (var->attr & EFI_VARIABLE_RUNTIME_ACCESS) {
> >>>>> - u16 *data;
> >>>>> -
> >>>>> - /* skip variable */
> >>>>> - for (data = var->name; *data; ++data)
> >>>>> - ;
> >>>>> - ++data;
> >>>>> - var = (struct efi_var_entry *)
> >>>>> - ALIGN((uintptr_t)data + var->length, 8);
> >>>>> - } else {
> >>>>> - /* delete variable */
> >>>>> - efi_var_mem_del(var);
> >>>>> - }
> >>>>> - }
> >>>>> -}
> >>>>> -
> >>>>> -/**
> >>>>> - * efi_var_mem_notify_exit_boot_services() - ExitBootService callback
> >>>>> - *
> >>>>> - * @event: callback event
> >>>>> - * @context: callback context
> >>>>> - */
> >>>>> -static void EFIAPI
> >>>>> -efi_var_mem_notify_exit_boot_services(struct efi_event *event, void *context)
> >>>>> -{
> >>>>> - EFI_ENTRY("%p, %p", event, context);
> >>>>> -
> >>>>> - /* Delete boot service only variables */
> >>>>> - efi_var_mem_bs_del();
> >>>>> -
> >>>>> - EFI_EXIT(EFI_SUCCESS);
> >>>>> -}
> >>>>> -
> >>>>> /**
> >>>>> * efi_var_mem_notify_exit_boot_services() - SetVirtualMemoryMap callback
> >>>>> *
> >>>>> @@ -261,11 +231,7 @@ efi_status_t efi_var_mem_init(void)
> >>>>> efi_var_buf->magic = EFI_VAR_FILE_MAGIC;
> >>>>> efi_var_buf->length = (uintptr_t)efi_var_buf->var -
> >>>>> (uintptr_t)efi_var_buf;
> >>>>> - /* crc32 for 0 bytes = 0 */
> >>>>>
> >>>>> - ret = efi_create_event(EVT_SIGNAL_EXIT_BOOT_SERVICES, TPL_CALLBACK,
> >>>>> - efi_var_mem_notify_exit_boot_services, NULL,
> >>>>> - NULL, &event);
> >>>>> if (ret != EFI_SUCCESS)
> >>>>> return ret;
> >>>>> ret = efi_create_event(EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, TPL_CALLBACK,
> >>>>> @@ -276,10 +242,71 @@ efi_status_t efi_var_mem_init(void)
> >>>>> return ret;
> >>>>> }
> >>>>>
> >>>>> +/**
> >>>>> + * efi_var_collect_mem() - Copy EFI variables matching attributes mask from
> >>>>> + * efi_var_buf
> >>>>> + *
> >>>>> + * @buf: buffer containing variable collection
> >>>>> + * @lenp: buffer length
> >>>>> + * @mask: mask of matched attributes
> >>>>> + *
> >>>>> + * Return: Status code
> >>>>> + */
> >>>>> +efi_status_t __efi_runtime
> >>>>> +efi_var_collect_mem(struct efi_var_file *buf, efi_uintn_t *lenp, u32 mask)
> >>>>> +{
> >>>>> + static struct efi_var_file __efi_runtime_data hdr = {
> >>>>> + .magic = EFI_VAR_FILE_MAGIC,
> >>>>> + };
> >>>>> + struct efi_var_entry *last, *var, *var_to;
> >>>>> +
> >>>>> + hdr.length = sizeof(struct efi_var_file);
> >>>>> +
> >>>>> + var = efi_var_buf->var;
> >>>>> + last = (struct efi_var_entry *)
> >>>>> + ((uintptr_t)efi_var_buf + efi_var_buf->length);
> >>>>> + if (buf)
> >>>>> + var_to = buf->var;
> >>>>> +
> >>>>> + while (var < last) {
> >>>>> + u32 len = efi_var_entry_len(var);
> >>>>> +
> >>>>> + if ((var->attr & mask) != mask) {
> >>>>> + var = (void *)((uintptr_t)var + len);
> >>>>> + continue;
> >>>>> + }
> >>>>> +
> >>>>> + hdr.length += len;
> >>>>> +
> >>>>> + if (buf && hdr.length <= *lenp) {
> >>>>> + efi_memcpy_runtime(var_to, var, len);
> >>>>> + var_to = (void *)var_to + len;
> >>>>> + }
> >>>>> + var = (void *)var + len;
> >>>>> + }
> >>>>> +
> >>>>> + if (!buf && hdr.length <= *lenp) {
> >>>>> + *lenp = hdr.length;
> >>>>> + return EFI_INVALID_PARAMETER;
> >>>>> + }
> >>>>> +
> >>>>> + if (!buf || hdr.length > *lenp) {
> >>>>> + *lenp = hdr.length;
> >>>>> + return EFI_BUFFER_TOO_SMALL;
> >>>>> + }
> >>>>> + hdr.crc32 = crc32(0, (u8 *)buf->var,
> >>>>> + hdr.length - sizeof(struct efi_var_file));
> >>>>> +
> >>>>> + efi_memcpy_runtime(buf, &hdr, sizeof(hdr));
> >>>>> + *lenp = hdr.length;
> >>>>> +
> >>>>> + return EFI_SUCCESS;
> >>>>> +}
> >>>>> +
> >>>>> efi_status_t __efi_runtime
> >>>>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>>>> - u64 *timep)
> >>>>> + u64 *timep, u32 mask)
> >>>>> {
> >>>>> efi_uintn_t old_size;
> >>>>> struct efi_var_entry *var;
> >>>>> @@ -291,11 +318,22 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>>> if (!var)
> >>>>> return EFI_NOT_FOUND;
> >>>>>
> >>>>> + /*
> >>>>> + * This function is used at runtime to dump EFI variables.
> >>>>> + * The memory backend we keep around has BS-only variables as
> >>>>> + * well. At runtime we filter them here
> >>>>> + */
> >>>>> + if (mask && !((var->attr & mask) == mask))
> >>>>> + return EFI_NOT_FOUND;
> >>>>> +
> >>>>> if (attributes)
> >>>>> *attributes = var->attr;
> >>>>> if (timep)
> >>>>> *timep = var->time;
> >>>>>
> >>>>> + if (!u16_strcmp(variable_name, u"VarToFile"))
> >>>>> + return efi_var_collect_mem(data, data_size, EFI_VARIABLE_NON_VOLATILE);
> >>>>> +
> >>>>> old_size = *data_size;
> >>>>> *data_size = var->length;
> >>>>> if (old_size < var->length)
> >>>>> @@ -315,7 +353,8 @@ efi_get_variable_mem(const u16 *variable_name, const efi_guid_t *vendor,
> >>>>>
> >>>>> efi_status_t __efi_runtime
> >>>>> efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>>>> - u16 *variable_name, efi_guid_t *vendor)
> >>>>> + u16 *variable_name, efi_guid_t *vendor,
> >>>>> + u32 mask)
> >>>>> {
> >>>>> struct efi_var_entry *var;
> >>>>> efi_uintn_t len, old_size;
> >>>>> @@ -324,6 +363,7 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>>>> if (!variable_name_size || !variable_name || !vendor)
> >>>>> return EFI_INVALID_PARAMETER;
> >>>>>
> >>>>> +skip:
> >>>>> len = *variable_name_size >> 1;
> >>>>> if (u16_strnlen(variable_name, len) == len)
> >>>>> return EFI_INVALID_PARAMETER;
> >>>>> @@ -347,6 +387,11 @@ efi_get_next_variable_name_mem(efi_uintn_t *variable_name_size,
> >>>>> efi_memcpy_runtime(variable_name, var->name, *variable_name_size);
> >>>>> efi_memcpy_runtime(vendor, &var->guid, sizeof(efi_guid_t));
> >>>>>
> >>>>> + if (mask && !((var->attr & mask) == mask)) {
> >>>>> + *variable_name_size = old_size;
> >>>>> + goto skip;
> >>>>> + }
> >>>>> +
> >>>>> return EFI_SUCCESS;
> >>>>> }
> >>>>>
> >>>>> diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> >>>>> index 47bb79920575..0cbed53d1dbf 100644
> >>>>> --- a/lib/efi_loader/efi_variable.c
> >>>>> +++ b/lib/efi_loader/efi_variable.c
> >>>>> @@ -209,14 +209,16 @@ efi_get_variable_int(const u16 *variable_name, const efi_guid_t *vendor,
> >>>>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>>>> u64 *timep)
> >>>>> {
> >>>>> - return efi_get_variable_mem(variable_name, vendor, attributes, data_size, data, timep);
> >>>>> + return efi_get_variable_mem(variable_name, vendor, attributes, data_size,
> >>>>> + data, timep, 0);
> >>>>> }
> >>>>>
> >>>>> efi_status_t __efi_runtime
> >>>>> efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> >>>>> u16 *variable_name, efi_guid_t *vendor)
> >>>>> {
> >>>>> - return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> >>>>> + return efi_get_next_variable_name_mem(variable_name_size, variable_name,
> >>>>> + vendor, 0);
> >>>>> }
> >>>>>
> >>>>> /**
> >>>>> diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> >>>>> index dde135fd9f81..4f1aa298da13 100644
> >>>>> --- a/lib/efi_loader/efi_variable_tee.c
> >>>>> +++ b/lib/efi_loader/efi_variable_tee.c
> >>>>> @@ -959,11 +959,6 @@ void efi_variables_boot_exit_notify(void)
> >>>>> log_err("Unable to notify the MM partition for ExitBootServices\n");
> >>>>> free(comm_buf);
> >>>>>
> >>>>> - /*
> >>>>> - * Populate the list for runtime variables.
> >>>>> - * asking EFI_VARIABLE_RUNTIME_ACCESS is redundant, since
> >>>>> - * efi_var_mem_notify_exit_boot_services will clean those, but that's fine
> >>>>> - */
> >>>>> ret = efi_var_collect(&var_buf, &len, EFI_VARIABLE_RUNTIME_ACCESS);
> >>>>> if (ret != EFI_SUCCESS)
> >>>>> log_err("Can't populate EFI variables. No runtime variables will be available\n");
> >>>>> --
> >>>>> 2.40.1
> >>>>>
> >>>>>
> >>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/4] efi_selftest: add tests for setvariableRT
2024-04-18 12:54 ` [PATCH v3 4/4] efi_selftest: add tests for setvariableRT Ilias Apalodimas
@ 2024-04-20 7:17 ` Heinrich Schuchardt
2024-04-20 11:56 ` Ilias Apalodimas
0 siblings, 1 reply; 15+ messages in thread
From: Heinrich Schuchardt @ 2024-04-20 7:17 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Tom Rini, Masahisa Kojima, Raymond Mao, Simon Glass,
Janne Grunau, Matthias Schiffer, Abdellatif El Khlifi,
Alper Nebi Yasak, Sughosh Ganu, Sam Edwards, Richard Henderson,
Weizhao Ouyang, u-boot, kettenis, AKASHI Takahiro
On 4/18/24 14:54, Ilias Apalodimas wrote:
> Since we support SetVariableRT now add the relevant tests
>
> - Search for the RTStorageVolatile and VarToFile variables after EBS
> - Try to update with invalid variales (BS, RT only)
> - Try to write a variable bigger than our backend storage
> - Write a variable that fits and check VarToFile has been updated
> correclty
> - Append to the variable and check VarToFile changes
> - Try to delete VarToFile which is write protected
> - Try to add/delete runtime variables
> - Verify VarToFile contains a valid file format
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> .../efi_selftest_variables_runtime.c | 197 +++++++++++++++++-
> 1 file changed, 196 insertions(+), 1 deletion(-)
>
> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> index 986924b881dd..28b4e9e7afa5 100644
> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> @@ -10,6 +10,8 @@
> */
>
> #include <efi_selftest.h>
> +#include <efi_variable.h>
> +#include <u-boot/crc.h>
>
> #define EFI_ST_MAX_DATA_SIZE 16
> #define EFI_ST_MAX_VARNAME_SIZE 40
> @@ -17,6 +19,8 @@
> static struct efi_boot_services *boottime;
> static struct efi_runtime_services *runtime;
> static const efi_guid_t guid_vendor0 = EFI_GLOBAL_VARIABLE_GUID;
> +static const efi_guid_t __efi_runtime_data efi_rt_var_guid =
> + U_BOOT_EFI_RT_VAR_FILE_GUID;
>
> /*
> * Setup unit test.
> @@ -41,15 +45,18 @@ static int setup(const efi_handle_t img_handle,
> static int execute(void)
> {
> efi_status_t ret;
> - efi_uintn_t len;
> + efi_uintn_t len, avail, append_len = 17;
> u32 attr;
> u8 v[16] = {0x5d, 0xd1, 0x5e, 0x51, 0x5a, 0x05, 0xc7, 0x0c,
> 0x35, 0x4a, 0xae, 0x87, 0xa5, 0xdf, 0x0f, 0x65,};
> + u8 v2[CONFIG_EFI_VAR_BUF_SIZE];
> u8 data[EFI_ST_MAX_DATA_SIZE];
> + u8 data2[CONFIG_EFI_VAR_BUF_SIZE];
> u16 varname[EFI_ST_MAX_VARNAME_SIZE];
> efi_guid_t guid;
> u64 max_storage, rem_storage, max_size;
>
> + memset(v2, 0x1, sizeof(v2));
> ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> &max_storage, &rem_storage,
> &max_size);
> @@ -63,11 +70,199 @@ static int execute(void)
> EFI_VARIABLE_RUNTIME_ACCESS,
> 3, v + 4);
> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> + efi_uintn_t prev_len, delta;
> + struct efi_var_entry *var;
> + struct efi_var_file *hdr;
> +
> /* At runtime only non-volatile variables may be set. */
> if (ret != EFI_INVALID_PARAMETER) {
> efi_st_error("SetVariable failed\n");
> return EFI_ST_FAILURE;
> }
> +
> + /* runtime atttribute must be set */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + 3, v + 4);
> + if (ret != EFI_INVALID_PARAMETER) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + len = sizeof(data);
> + ret = runtime->get_variable(u"RTStorageVolatile",
> + &efi_rt_var_guid,
> + &attr, &len, data);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("GetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
We should check data against EFI_VAR_FILE_NAME.
+ if (len != sizeof(EFI_VAR_FILE_NAME) ||
+ memcmp(data, EFI_VAR_FILE_NAME,
+ sizeof(EFI_VAR_FILE_NAME))) {
+ data[len - 1] = 0;
+ efi_st_error("RTStorageVolatile = %s\n", data);
+ return EFI_ST_FAILURE;
+ }
+
(memcmp() because we want to be able to carve out efi_selftest as a
standalone binary in future).
> +
> + len = sizeof(data2);
> + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> + &attr, &len, data2);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("GetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> + /*
> + * VarToFile size must change once a variable is inserted
> + * Store it now, we'll use it later
> + */
> + prev_len = len;
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + sizeof(v2),
> + v2);
> + /*
> + * This will try to update VarToFile as well and must fail,
> + * without changing or deleting VarToFile
> + */
> + if (ret != EFI_OUT_OF_RESOURCES) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> + len = sizeof(data2);
> + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> + &attr, &len, data2);
> + if (ret != EFI_SUCCESS || prev_len != len) {
> + efi_st_error("Get/SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Add an 8byte aligned variable */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + sizeof(v), v);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Delete it by setting the attrs to 0 */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + 0, sizeof(v), v);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Add it back */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + sizeof(v), v);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Delete it again by setting the size to 0 */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + 0, NULL);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Delete it again and make sure it's not there */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + 0, NULL);
> + if (ret != EFI_NOT_FOUND) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /*
> + * Add a non-aligned variable
> + * VarToFile updates must include efi_st_var0
> + */
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + 9, v + 4);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> + var = efi_var_mem_find(&guid_vendor0, u"efi_st_var0", NULL);
> + if (!var) {
> + efi_st_error("GetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> + delta = efi_var_entry_len(var);
> + len = sizeof(data2);
> + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> + &attr, &len, data2);
> + if (ret != EFI_SUCCESS || prev_len + delta != len) {
Please, provide separate messages for length and GetVariable.
ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
&attr, &len, data2);
if (ret != EFI_SUCCESS) {
efi_st_error("GetVariable failed\n");
return EFI_ST_FAILURE;
}
if (prev_len + delta != len) {
efi_st_error("Unexpected VarToFile size"),
return EFI_ST_FAILURE;
}
Best regards
Heinrich
> + efi_st_error("Get/SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /*
> + * Append on an existing variable must update VarToFile
> + * Our variable entries are 8-byte aligned.
> + * Adding a single byte will fit on the existing space
> + */
> + prev_len = len;
> + avail = efi_var_entry_len(var) -
> + (sizeof(u16) * (u16_strlen(var->name) + 1) + sizeof(*var)) -
> + var->length;
> + if (avail >= append_len)
> + delta = 0;
> + else
> + delta = ALIGN(append_len - avail, 8);
> + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_APPEND_WRITE |
> + EFI_VARIABLE_NON_VOLATILE,
> + append_len, v2);
> + if (ret != EFI_SUCCESS) {
> + efi_st_error("SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> + len = sizeof(data2);
> + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> + &attr, &len, data2);
> + if (ret != EFI_SUCCESS || prev_len + delta != len) {
> + efi_st_error("Get/SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Make sure that variable contains a valid file */
> + hdr = (struct efi_var_file *)data2;
> + if (hdr->magic != EFI_VAR_FILE_MAGIC ||
> + len != hdr->length ||
> + hdr->crc32 != crc32(0, (u8 *)((uintptr_t)data2 + sizeof(struct efi_var_file)),
> + len - sizeof(struct efi_var_file))) {
> + efi_st_error("VarToFile invalid header\n");
> + return EFI_ST_FAILURE;
> + }
> +
> + /* Variables that are BS, RT and volatile are RO after EBS */
> + ret = runtime->set_variable(u"VarToFile", &efi_rt_var_guid,
> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> + EFI_VARIABLE_RUNTIME_ACCESS |
> + EFI_VARIABLE_NON_VOLATILE,
> + sizeof(v), v);
> + if (ret != EFI_WRITE_PROTECTED) {
> + efi_st_error("Get/SetVariable failed\n");
> + return EFI_ST_FAILURE;
> + }
> } else {
> if (ret != EFI_UNSUPPORTED) {
> efi_st_error("SetVariable failed\n");
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/4]
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
` (3 preceding siblings ...)
2024-04-18 12:54 ` [PATCH v3 4/4] efi_selftest: add tests for setvariableRT Ilias Apalodimas
@ 2024-04-20 7:20 ` Heinrich Schuchardt
2024-04-20 11:57 ` Ilias Apalodimas
4 siblings, 1 reply; 15+ messages in thread
From: Heinrich Schuchardt @ 2024-04-20 7:20 UTC (permalink / raw)
To: Ilias Apalodimas
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Tom Rini, Masahisa Kojima, Raymond Mao,
Matthias Schiffer, Simon Glass, Janne Grunau,
Abdellatif El Khlifi, Sughosh Ganu, Richard Henderson,
Sam Edwards, Alper Nebi Yasak, Weizhao Ouyang, u-boot,
AKASHI Takahiro, kettenis
On 4/18/24 14:54, Ilias Apalodimas wrote:
> Hi!
> This is v3 of SetVariable at runtime [0]
>
> Nothing changed drastically from v2.
> A few more test cases have been added, comments/suggestions have been
> addressed and a bug where deleting a variable by setting 'attributes' to
> 0 has been fixed.
>
> Changes since v2:
> - Add more selftests checking for variable deletion as well as the
> VarToFile header format
> - Use unaligned sized variables on tests
> - Add a new function to get the variable entry length instead of
> repurposing efi_var_mem_compare()
> - Converted VarToFile to RO
> - Added a few comments requested by Heinrich
> - Fixed a bug where SetVariable with attributes set to 0 did not delete
> the variable but returned EFI_INVALID_PARAMETER instead
> - Run FWTS 'uefitests' and attach the log in patch #1
> - Added r-b tags from Heinrich
>
> Changes since v1:
> - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
> This allows us to get rid of the preallocated RT buffer, since the
> address is user provided
> - convert Set/GetVariableRT -> Set/GetVariable at runtime
> - return EFI_INVALID_PARAM is NV is not set at runtime
> - Heinrich sent me the efi_var_collect_mem() variant
>
> Changes since the RFC:
> - Return EFI_INVALID_PARAM if attributes are not volatile
> - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> - Add 2 EFI variables and allow userspace to write the file
> - Add selftests
>
> I also have a patch enable QueryVariableInfo [1], but I don't want to
> introduce new patches now. This also needs a few more testcases of its
> own so I'll send it once we finalize this one.
>
> [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
>
> Regards
> /Ilias
>
> Ilias Apalodimas (4):
> efi_loader: conditionally enable SetvariableRT
> efi_loader: Add OS notifications for SetVariable at runtime
> efi_loader: add an EFI variable with the file contents
> efi_selftest: add tests for setvariableRT
I am missing a defconfig change which is needed to run the unit test in
Gitlab CI. I would prefer testing this on the sandbox.
Best regards
Heinrich
>
> include/efi_loader.h | 4 +
> include/efi_variable.h | 16 +-
> lib/charset.c | 2 +-
> lib/efi_loader/Kconfig | 16 ++
> lib/efi_loader/efi_runtime.c | 42 ++++
> lib/efi_loader/efi_var_common.c | 6 +-
> lib/efi_loader/efi_var_mem.c | 151 ++++++++-----
> lib/efi_loader/efi_variable.c | 122 ++++++++--
> lib/efi_loader/efi_variable_tee.c | 5 -
> .../efi_selftest_variables_runtime.c | 211 +++++++++++++++++-
> 10 files changed, 495 insertions(+), 80 deletions(-)
>
> --
> 2.40.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 4/4] efi_selftest: add tests for setvariableRT
2024-04-20 7:17 ` Heinrich Schuchardt
@ 2024-04-20 11:56 ` Ilias Apalodimas
0 siblings, 0 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-20 11:56 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Tom Rini, Masahisa Kojima, Raymond Mao, Simon Glass,
Janne Grunau, Matthias Schiffer, Abdellatif El Khlifi,
Alper Nebi Yasak, Sughosh Ganu, Sam Edwards, Richard Henderson,
Weizhao Ouyang, u-boot, kettenis, AKASHI Takahiro
Hi Heinrich,
I was about to fix and send a v4, but I see you fixed them up on the PR.
Thanks!
On Sat, 20 Apr 2024 at 10:23, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/18/24 14:54, Ilias Apalodimas wrote:
> > Since we support SetVariableRT now add the relevant tests
> >
> > - Search for the RTStorageVolatile and VarToFile variables after EBS
> > - Try to update with invalid variales (BS, RT only)
> > - Try to write a variable bigger than our backend storage
> > - Write a variable that fits and check VarToFile has been updated
> > correclty
> > - Append to the variable and check VarToFile changes
> > - Try to delete VarToFile which is write protected
> > - Try to add/delete runtime variables
> > - Verify VarToFile contains a valid file format
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> > .../efi_selftest_variables_runtime.c | 197 +++++++++++++++++-
> > 1 file changed, 196 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > index 986924b881dd..28b4e9e7afa5 100644
> > --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> > +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -10,6 +10,8 @@
> > */
> >
> > #include <efi_selftest.h>
> > +#include <efi_variable.h>
> > +#include <u-boot/crc.h>
> >
> > #define EFI_ST_MAX_DATA_SIZE 16
> > #define EFI_ST_MAX_VARNAME_SIZE 40
> > @@ -17,6 +19,8 @@
> > static struct efi_boot_services *boottime;
> > static struct efi_runtime_services *runtime;
> > static const efi_guid_t guid_vendor0 = EFI_GLOBAL_VARIABLE_GUID;
> > +static const efi_guid_t __efi_runtime_data efi_rt_var_guid =
> > + U_BOOT_EFI_RT_VAR_FILE_GUID;
> >
> > /*
> > * Setup unit test.
> > @@ -41,15 +45,18 @@ static int setup(const efi_handle_t img_handle,
> > static int execute(void)
> > {
> > efi_status_t ret;
> > - efi_uintn_t len;
> > + efi_uintn_t len, avail, append_len = 17;
> > u32 attr;
> > u8 v[16] = {0x5d, 0xd1, 0x5e, 0x51, 0x5a, 0x05, 0xc7, 0x0c,
> > 0x35, 0x4a, 0xae, 0x87, 0xa5, 0xdf, 0x0f, 0x65,};
> > + u8 v2[CONFIG_EFI_VAR_BUF_SIZE];
> > u8 data[EFI_ST_MAX_DATA_SIZE];
> > + u8 data2[CONFIG_EFI_VAR_BUF_SIZE];
> > u16 varname[EFI_ST_MAX_VARNAME_SIZE];
> > efi_guid_t guid;
> > u64 max_storage, rem_storage, max_size;
> >
> > + memset(v2, 0x1, sizeof(v2));
> > ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> > &max_storage, &rem_storage,
> > &max_size);
> > @@ -63,11 +70,199 @@ static int execute(void)
> > EFI_VARIABLE_RUNTIME_ACCESS,
> > 3, v + 4);
> > if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > + efi_uintn_t prev_len, delta;
> > + struct efi_var_entry *var;
> > + struct efi_var_file *hdr;
> > +
> > /* At runtime only non-volatile variables may be set. */
> > if (ret != EFI_INVALID_PARAMETER) {
> > efi_st_error("SetVariable failed\n");
> > return EFI_ST_FAILURE;
> > }
> > +
> > + /* runtime atttribute must be set */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + 3, v + 4);
> > + if (ret != EFI_INVALID_PARAMETER) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + len = sizeof(data);
> > + ret = runtime->get_variable(u"RTStorageVolatile",
> > + &efi_rt_var_guid,
> > + &attr, &len, data);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("GetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
>
> We should check data against EFI_VAR_FILE_NAME.
>
> + if (len != sizeof(EFI_VAR_FILE_NAME) ||
> + memcmp(data, EFI_VAR_FILE_NAME,
> + sizeof(EFI_VAR_FILE_NAME))) {
> + data[len - 1] = 0;
> + efi_st_error("RTStorageVolatile = %s\n", data);
> + return EFI_ST_FAILURE;
> + }
> +
>
> (memcmp() because we want to be able to carve out efi_selftest as a
> standalone binary in future).
>
> > +
> > + len = sizeof(data2);
> > + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > + &attr, &len, data2);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("GetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + /*
> > + * VarToFile size must change once a variable is inserted
> > + * Store it now, we'll use it later
> > + */
> > + prev_len = len;
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + sizeof(v2),
> > + v2);
> > + /*
> > + * This will try to update VarToFile as well and must fail,
> > + * without changing or deleting VarToFile
> > + */
> > + if (ret != EFI_OUT_OF_RESOURCES) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + len = sizeof(data2);
> > + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > + &attr, &len, data2);
> > + if (ret != EFI_SUCCESS || prev_len != len) {
> > + efi_st_error("Get/SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Add an 8byte aligned variable */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + sizeof(v), v);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Delete it by setting the attrs to 0 */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + 0, sizeof(v), v);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Add it back */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + sizeof(v), v);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Delete it again by setting the size to 0 */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + 0, NULL);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Delete it again and make sure it's not there */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + 0, NULL);
> > + if (ret != EFI_NOT_FOUND) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /*
> > + * Add a non-aligned variable
> > + * VarToFile updates must include efi_st_var0
> > + */
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + 9, v + 4);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + var = efi_var_mem_find(&guid_vendor0, u"efi_st_var0", NULL);
> > + if (!var) {
> > + efi_st_error("GetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + delta = efi_var_entry_len(var);
> > + len = sizeof(data2);
> > + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > + &attr, &len, data2);
> > + if (ret != EFI_SUCCESS || prev_len + delta != len) {
>
> Please, provide separate messages for length and GetVariable.
>
> ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> &attr, &len, data2);
> if (ret != EFI_SUCCESS) {
> efi_st_error("GetVariable failed\n");
> return EFI_ST_FAILURE;
> }
> if (prev_len + delta != len) {
> efi_st_error("Unexpected VarToFile size"),
> return EFI_ST_FAILURE;
> }
>
> Best regards
>
> Heinrich
>
> > + efi_st_error("Get/SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /*
> > + * Append on an existing variable must update VarToFile
> > + * Our variable entries are 8-byte aligned.
> > + * Adding a single byte will fit on the existing space
> > + */
> > + prev_len = len;
> > + avail = efi_var_entry_len(var) -
> > + (sizeof(u16) * (u16_strlen(var->name) + 1) + sizeof(*var)) -
> > + var->length;
> > + if (avail >= append_len)
> > + delta = 0;
> > + else
> > + delta = ALIGN(append_len - avail, 8);
> > + ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_APPEND_WRITE |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + append_len, v2);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + len = sizeof(data2);
> > + ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > + &attr, &len, data2);
> > + if (ret != EFI_SUCCESS || prev_len + delta != len) {
> > + efi_st_error("Get/SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Make sure that variable contains a valid file */
> > + hdr = (struct efi_var_file *)data2;
> > + if (hdr->magic != EFI_VAR_FILE_MAGIC ||
> > + len != hdr->length ||
> > + hdr->crc32 != crc32(0, (u8 *)((uintptr_t)data2 + sizeof(struct efi_var_file)),
> > + len - sizeof(struct efi_var_file))) {
> > + efi_st_error("VarToFile invalid header\n");
> > + return EFI_ST_FAILURE;
> > + }
> > +
> > + /* Variables that are BS, RT and volatile are RO after EBS */
> > + ret = runtime->set_variable(u"VarToFile", &efi_rt_var_guid,
> > + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > + EFI_VARIABLE_RUNTIME_ACCESS |
> > + EFI_VARIABLE_NON_VOLATILE,
> > + sizeof(v), v);
> > + if (ret != EFI_WRITE_PROTECTED) {
> > + efi_st_error("Get/SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > } else {
> > if (ret != EFI_UNSUPPORTED) {
> > efi_st_error("SetVariable failed\n");
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v3 0/4]
2024-04-20 7:20 ` [PATCH v3 0/4] Heinrich Schuchardt
@ 2024-04-20 11:57 ` Ilias Apalodimas
0 siblings, 0 replies; 15+ messages in thread
From: Ilias Apalodimas @ 2024-04-20 11:57 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: caleb.connolly, sumit.garg, quic_llindhol, ardb, pbrobinson,
pjones, Tom Rini, Masahisa Kojima, Raymond Mao,
Matthias Schiffer, Simon Glass, Janne Grunau,
Abdellatif El Khlifi, Sughosh Ganu, Richard Henderson,
Sam Edwards, Alper Nebi Yasak, Weizhao Ouyang, u-boot,
AKASHI Takahiro, kettenis
On Sat, 20 Apr 2024 at 10:20, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 4/18/24 14:54, Ilias Apalodimas wrote:
> > Hi!
> > This is v3 of SetVariable at runtime [0]
> >
> > Nothing changed drastically from v2.
> > A few more test cases have been added, comments/suggestions have been
> > addressed and a bug where deleting a variable by setting 'attributes' to
> > 0 has been fixed.
> >
> > Changes since v2:
> > - Add more selftests checking for variable deletion as well as the
> > VarToFile header format
> > - Use unaligned sized variables on tests
> > - Add a new function to get the variable entry length instead of
> > repurposing efi_var_mem_compare()
> > - Converted VarToFile to RO
> > - Added a few comments requested by Heinrich
> > - Fixed a bug where SetVariable with attributes set to 0 did not delete
> > the variable but returned EFI_INVALID_PARAMETER instead
> > - Run FWTS 'uefitests' and attach the log in patch #1
> > - Added r-b tags from Heinrich
> >
> > Changes since v1:
> > - Instead of Creating VarToFile at SetVariable, create it on GetVariable.
> > This allows us to get rid of the preallocated RT buffer, since the
> > address is user provided
> > - convert Set/GetVariableRT -> Set/GetVariable at runtime
> > - return EFI_INVALID_PARAM is NV is not set at runtime
> > - Heinrich sent me the efi_var_collect_mem() variant
> >
> > Changes since the RFC:
> > - Return EFI_INVALID_PARAM if attributes are not volatile
> > - Add EFI_WRITE_PROTECTED checks for BS, RT *only* variables
> > - Add 2 EFI variables and allow userspace to write the file
> > - Add selftests
> >
> > I also have a patch enable QueryVariableInfo [1], but I don't want to
> > introduce new patches now. This also needs a few more testcases of its
> > own so I'll send it once we finalize this one.
> >
> > [0] https://lore.kernel.org/u-boot/20240417101928.119115-1-ilias.apalodimas@linaro.org/T/
> > [1] https://source.denx.de/u-boot/custodians/u-boot-tpm/-/commit/ce35270aaeb93686d7e013f3b028808e8af88cc0
> >
> > Regards
> > /Ilias
> >
> > Ilias Apalodimas (4):
> > efi_loader: conditionally enable SetvariableRT
> > efi_loader: Add OS notifications for SetVariable at runtime
> > efi_loader: add an EFI variable with the file contents
> > efi_selftest: add tests for setvariableRT
>
> I am missing a defconfig change which is needed to run the unit test in
> Gitlab CI. I would prefer testing this on the sandbox.
>
> Best regards
Ok I'll send a followup since you already sent a PR with these
Thanks!
/Ilias
>
> Heinrich
>
> >
> > include/efi_loader.h | 4 +
> > include/efi_variable.h | 16 +-
> > lib/charset.c | 2 +-
> > lib/efi_loader/Kconfig | 16 ++
> > lib/efi_loader/efi_runtime.c | 42 ++++
> > lib/efi_loader/efi_var_common.c | 6 +-
> > lib/efi_loader/efi_var_mem.c | 151 ++++++++-----
> > lib/efi_loader/efi_variable.c | 122 ++++++++--
> > lib/efi_loader/efi_variable_tee.c | 5 -
> > .../efi_selftest_variables_runtime.c | 211 +++++++++++++++++-
> > 10 files changed, 495 insertions(+), 80 deletions(-)
> >
> > --
> > 2.40.1
> >
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2024-04-20 11:58 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-04-18 12:54 [PATCH v3 0/4] Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 1/4] efi_loader: conditionally enable SetvariableRT Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 2/4] efi_loader: Add OS notifications for SetVariable at runtime Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 3/4] efi_loader: add an EFI variable with the file contents Ilias Apalodimas
2024-04-18 15:15 ` Mark Kettenis
2024-04-18 15:36 ` Ilias Apalodimas
2024-04-18 15:42 ` Heinrich Schuchardt
2024-04-18 15:59 ` Ilias Apalodimas
2024-04-18 16:14 ` Heinrich Schuchardt
2024-04-18 17:03 ` Ilias Apalodimas
2024-04-18 12:54 ` [PATCH v3 4/4] efi_selftest: add tests for setvariableRT Ilias Apalodimas
2024-04-20 7:17 ` Heinrich Schuchardt
2024-04-20 11:56 ` Ilias Apalodimas
2024-04-20 7:20 ` [PATCH v3 0/4] Heinrich Schuchardt
2024-04-20 11:57 ` Ilias Apalodimas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.