On Wed, 17 Apr 2024 at 15:28, Heinrich Schuchardt heinrich.schuchardt@canonical.com wrote:

On 17.04.24 12:19, Ilias Apalodimas wrote:

...

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

[0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-st...

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 | 115 ++++++++++++++++-- .../efi_selftest_variables_runtime.c | 13 +- 4 files changed, 134 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..abc2a3402f42 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,78 @@ 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;
  

• /* BS only variables are hidden deny writing them */
  

• if (!(attributes & EFI_VARIABLE_RUNTIME_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);
  

• 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..4c9405c0a7c7 100644 --- a/lib/efi_selftest/efi_selftest_variables_runtime.c +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c @@ -62,9 +62,16 @@ 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)) {
  

•         if (ret != EFI_INVALID_PARAMETER) {
  

A comment might be helpful here:

 /* At runtime only non-volatile variables may be set. */

Otherwise looks good to me.

Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

Thanks, Heinrich! I'll add the comment in v3

...

•                 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,
  

On 17.04.24 12:19, Ilias Apalodimas wrote:

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. |

Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

Reviewed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com

---

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

On 17.04.24 12:19, Ilias Apalodimas wrote:

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-st...

Co-developed-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Heinrich Schuchardt heinrich.schuchardt@canonical.com Signed-off-by: Ilias Apalodimas ilias.apalodimas@linaro.org

---

include/efi_variable.h | 14 ++- lib/charset.c | 2 +- lib/efi_loader/efi_runtime.c | 19 ++++ lib/efi_loader/efi_var_common.c | 6 +- lib/efi_loader/efi_var_mem.c | 146 ++++++++++++++++++------------ lib/efi_loader/efi_variable.c | 6 +- lib/efi_loader/efi_variable_tee.c | 5 - 7 files changed, 130 insertions(+), 68 deletions(-)

diff --git a/include/efi_variable.h b/include/efi_variable.h index 42a2b7c52bef..b545a36aac50 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,8 @@ 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);
  

• #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..99ad1f35d8f1 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)) {

• int s = 0;
  

u8 s = 0; should be enough.

• ret = efi_set_variable_int(u"RTStorageVolatile", &efi_guid_efi_rt_var_file, EFI_VARIABLE_BOOTSERVICE_ACCESS |

@@ -141,6 +143,23 @@ efi_status_t efi_init_runtime_supported(void) log_err("Failed to set RTStorageVolatile\n"); return ret; }

• ret = efi_set_variable_int(u"VarToFile",
  

• 			   &efi_guid_efi_rt_var_file,
  

• 			   EFI_VARIABLE_BOOTSERVICE_ACCESS |
  

• 			   EFI_VARIABLE_RUNTIME_ACCESS,
  

Why is the variable set here? A comment would be helpful. If you set it for protection, shouldn't it be EFI_VARIABLE_READ_ONLY?

An alternative would be to return EFI_INVALID_PARAMETER in efi_set_variable_int for any attempt to set a variable with GUID efi_guid_efi_rt_var_file.

• 			   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..65ab858c926e 100644 --- a/lib/efi_loader/efi_var_mem.c +++ b/lib/efi_loader/efi_var_mem.c @@ -36,9 +36,11 @@ efi_var_mem_compare(struct efi_var_entry *var, const efi_guid_t *guid, const u16 *data, *var_name; bool match = true;

• for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;

•     i < sizeof(efi_guid_t) && match; ++i)
  

• match = (guid1[i] == guid2[i]);
  

• if (guid) {

• for (guid1 = (u8 *)&var->guid, guid2 = (u8 *)guid, i = 0;
  

• 	i < sizeof(efi_guid_t) && match; ++i)
  

• 	match = (guid1[i] == guid2[i]);
  

• }

  for (data = var->name, var_name = name;; ++data) { if (match)

@@ -184,53 +186,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 +216,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 +227,75 @@ 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;
  

• struct efi_var_entry *var_next;
  

• efi_var_mem_compare(var, NULL, u"", &var_next);
  

On IRC you suggested to make u16_strlen __efi_runtime_code to replace this efi_var_mem_compare() call and avoid the modifications of said function. That might be more readable.

Best regards

Heinrich

• len = (uintptr_t)var_next - (uintptr_t)var;
  

• if ((var->attr & mask) != mask) {
  

• 	var = (void *)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 +307,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 +342,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 +352,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 +376,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 abc2a3402f42..4aaa05a617d7 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");

Hi Heinrich,

[...]

...

• memset(v2, 0x1, sizeof(v2));
  ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
                                     &max_storage, &rem_storage,
                                     &max_size);
  

@@ -63,10 +69,107 @@ 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;
  

•         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 will 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;
  

•         }
  

•         /* SetVariableRT updates VarToFile with 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,
  

•                                     sizeof(v), v);
  

It is fine to test with variable name size (24) and variable value size (16) both being multiples of 8. But this does not cover the generic case.

We already test SetVariable with a non-aligned variable at the start. I'll keep this as is and add append 1byte at the last test as you suggested.

...

•         if (ret != EFI_SUCCESS) {
  

•                 efi_st_error("SetVariable failed\n");
  

•                 return EFI_ST_FAILURE;
  

•         }
  

•         len = sizeof(data2);
  

•         delta = 2 * (u16_strlen(u"efi_st_var0") + 1) + sizeof(v) +
  

•                 sizeof(struct efi_var_entry);
  

In the file all variable are stored at 8 byte aligned positions. I am missing ALIGN(,8) here.

Ah yes

...

•         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;
  

•         }
  

We should check the header fields of VarToFile (reserved, magic, length, crc32), e.g.

struct efi_var_file *hdr = (void *)data2; if (memcmp(hdr->magic, EFI_VAR_FILE_MAGIC, 8) || len != ALIGN(hdr->length + sizeof(hdr), 8) || ... )

Sure, good idea,

...

•         /* append on an existing variable will updateVarToFile */
  

•         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,
  

•                                     sizeof(v), v);
  

How about just appending 1 byte when using EFI_VARIABLE_APPEND_WRITE and checking that the delta here too?

Yes, I'll change this

[...]

Thanks /Ilias