On Wed, 17 Apr 2024 at 15:28, Heinrich Schuchardt <heinrich.schucha...@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-storage > > > > Signed-off-by: Ilias Apalodimas <ilias.apalodi...@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.schucha...@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, >