Re: [RFC PATCH] efi_loader: conditionally enable SetvariableRT

[...]

...

+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 storate. The OS will be responsible for syncing
  

Hello Ilias,

I like the idea of adding this for experimenting. We should express that this is just experimental and may be removed in future releases, if we find a better solution.

U-Boot documentation is good enough? I've cc'ed Peter (which maintain efivar AFAIK). If we agree on how to teach efivar/efibootmgr to use that file, I can describe it in docs

%s/storate/stroage/

...

•   the RAM contents to the file, otherwise any changes made during
  

•   runtime won't persist reboots.
  

•   Authenticated variables are not supported.
  

Should we mention here that this behavior is not compliant with the UEFI specification?

Should we unset CONFIG_EFI_EBBR_2_1_CONFORMANCE if this flag is set?

No, I don't think so. I'll ask around to make sure. However, EBBR already standardizes the file format and explicitly prevents storing authenticated variables. There's a separate test suite from arm called SIE (SEcurity interface extensions) which is complementary to SystemReady-IR and tests EFI authenticated variables.

...

• 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 18da6892e796..b38ce239e2d2 100644 --- a/lib/efi_loader/efi_runtime.c +++ b/lib/efi_loader/efi_runtime.c @@ -126,6 +126,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 40f7a0fb10d5..3b770ff16971 100644 --- a/lib/efi_loader/efi_variable.c +++ b/lib/efi_loader/efi_variable.c @@ -218,17 +218,21 @@ 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)
  

+/**

• • efi_check_setvariable() - 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 EFIAPI +efi_check_setvariable(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;
  
  

@@ -256,6 +260,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 = efi_check_setvariable(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);
  

@@ -434,7 +457,72 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor, u32 attributes, efi_uintn_t data_size, const void *data) { +#if IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)

Please, use 'if' instead of #if. The

Sure

...

• struct efi_var_entry *var;
  

• efi_uintn_t ret;
  

• bool append, delete;
  

• u64 time = 0;
  

• /* Authenticated variables are not supported */
  

• if (attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)
  

Adding EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS here would make it clearer that all authenticated variables are unsupported.

Ok, will do

Non-volatile variables are read-only at runtime. Please, check that EFI_VARIABLE_NON_VOLATILE is set.

You mean volatile right? But yea this needs fixing

if ((attributes & ( EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS | EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS | EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS)) || (~attributes & EFI_VARIABLE_NON_VOLATILE) return EFI_INVALID_PARAMETER;

...

•                 return EFI_INVALID_PARAMETER;
  

• ret = efi_check_setvariable(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 &= ~(u32)EFI_VARIABLE_APPEND_WRITE;
  

Why is the u32 conversion necessary? The result should be the same without conversion.

The UEFI specification defines the different attribute constants as 32bit. Why do we define them as 64bit in include/efi.h?

I guess this is an existing misinterpretation of the spec. I copy pasted this check from efi_set_variable_int(). It seems we should fix both. I'll include the fix in v2

...

• delete = !append && (!data_size || !attributes);
  

• if (var) {
  

•         if (var->attr & EFI_VARIABLE_READ_ONLY)
  

•                 return EFI_WRITE_PROTECTED;
  

•         /* attributes won't be changed */
  

•         if (!delete && (((var->attr & ~(u32)EFI_VARIABLE_READ_ONLY) !=
  

•             (attributes & ~(u32)EFI_VARIABLE_READ_ONLY))))
  

•                 return EFI_INVALID_PARAMETER;
  

ditto

...

•         time = var->time;
  

• } else {
  

•         if (delete || append)
  

EDK II allows to create a variable with EFI_VARIABLE_APPEND_WRITE. See previous discussions about changing this at boot time.

Yes, the reason I kept it like this is that we didn't merge that yet and I wanted to keep the same behavior. I'll update this one

[...]

Thanks /Ilias