On Fri, 29 Mar 2024 at 14:57, Heinrich Schuchardt <xypron.g...@gmx.de> wrote: > > On 3/29/24 13:25, Mark Kettenis wrote: > >> From: Ilias Apalodimas <ilias.apalodi...@linaro.org> > >> Date: Fri, 29 Mar 2024 09:19:27 +0200 > >> > >> When EFI variables are stored on file we don't allow SetVariableRT, > >> 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 GetVariableRT we copy runtime variables in RAM and expose them to > >> the OS. Add a Kconfig option and provide SetVariableRT 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 > >> > >> Open questions: > >> Looking at the EFI spec, I can't find a documented way of notifying the OS > >> that the storage is volatile. I would like to send a hint to the OS about > >> that and I was thinking of adding a configuration table with the filename, > >> which U-Boot expects to be on the ESP. Alternatively we could add a device > >> path which would be a bit harder to parse from the userspace application, > >> but safer in case multiple ESPs are present. > > > > Should there be a way for the OS to opt-in to this new behaviour? > > With the patch you could handle persisting variables in user space > without any modifications of the OS. E.g. use fanotify to watch all > efivarfs mounts in Linux.
Even better er can teach efivar etc to do this, which is going to be seamless for distros > > Ilias has also been considering a Linux driver for file based variables > inside the OS but then you wouldn't need this patch. Yea, I changed my mind on that upcall. I don't think it's elegant and it's going to be linux specific. The reason for doing what this patch propose, is get wider OS adoption. > > > > >> Since I am not too familiar with how BSDs treat and expose config tables, > >> we could instead add a RO efi variable with identical semantics, which > >> would be easier to read from userspace. > > > > There is an EFIIOC_GET_TABLE ioctl that allows retreiving a table by GUID. > > This seems to be BSD specific. > > @Ilias: How would you read a configuration from user space in Linux? Linux exposes some config tables in sys/. e.g ./firmware/efi/esrt/. $~ ls /sys/firmware/efi/esrt/ entries fw_resource_count fw_resource_count_max fw_resource_version I think we would have to add code for that new special table, unless I am missing something. /Ilias > > Best regards > > Heinrich > > > > > > >> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org> > >> --- > >> lib/efi_loader/Kconfig | 14 +++ > >> lib/efi_loader/efi_runtime.c | 4 + > >> lib/efi_loader/efi_variable.c | 108 ++++++++++++++++-- > >> .../efi_selftest_variables_runtime.c | 13 ++- > >> 4 files changed, 126 insertions(+), 13 deletions(-) > >> > >> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig > >> index a7c3e05c13a0..c7f7383189e5 100644 > >> --- a/lib/efi_loader/Kconfig > >> +++ b/lib/efi_loader/Kconfig > >> @@ -63,6 +63,20 @@ 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 storate. 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. > >> + > >> 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) > >> + 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) > >> + 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; > >> + 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; > >> + time = var->time; > >> + } else { > >> + if (delete || append) > >> + /* > >> + * Trying to delete or to update a non-existent > >> + * variable. > >> + */ > >> + return EFI_NOT_FOUND; > >> + } > >> + > >> + if (delete) { > >> + /* EFI_NOT_FOUND has been handled before */ > >> + attributes = var->attr; > >> + ret = EFI_SUCCESS; > >> + } else if (append) { > >> + u16 *old_data = var->name; > >> + > >> + for (; *old_data; ++old_data) > >> + ; > >> + ++old_data; > >> + 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; > >> +#else > >> return EFI_UNSUPPORTED; > >> +#endif > >> } > >> > >> /** > >> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c > >> b/lib/efi_selftest/efi_selftest_variables_runtime.c > >> index 4700d9424105..6001b44989c8 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_SUCCESS) { > >> + 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.37.2 > >> > >> >