| Message ID | 20240329071929.2461441-1-ilias.apalodimas@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] efi_loader: conditionally enable SetvariableRT | expand |
+cc Peter On Fri, 29 Mar 2024 at 09:19, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > 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. Replying to myself here, but after a discussion with Ard we could - Add an EFI RO variable which contains the filename in the ESP u-boot used to store the variables - teach efivar/efibootmgr how to write that file - When a SetVariable takes place, we first write the file and then update the RAM backend That's nice because distros won't have to do anything to enable it apart from updating to newer versions of those binaries. We also discussed having an entire copy of the variables in that RO variable. That would make userspace life easier, as it would just have to 'dd' that variable in the file. This is a bit trickier than it sounds because the firmware needs to reason about the variables that go in that 'megavariable' (only boottime/runtime, non-volatiles ones), but it's still doable. > > 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. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 >
On 3/29/24 08:19, Ilias Apalodimas wrote: > 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. > > 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. > > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 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. %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? > + > 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 > + 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. Non-volatile variables are read-only at runtime. Please, check that EFI_VARIABLE_NON_VOLATILE is set. 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? > + 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. Best regards Heinrich > + /* > + * 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 >
> From: Ilias Apalodimas <ilias.apalodimas@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? > 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. > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 > >
[...] > > > > +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
Hi Mark, [...] > > > > 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? The feature itself is in a Kconfig option. If we agree on a way to notify the OS that the variable storage is in RAM and hence volatile, the OS is free to ignore it I guess > > > 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. Ah thanks. I am leaning towards an EFI variable now to signal the OS [...] /Ilias
Hi Heinrich, > > > > > > + 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. Looking at this again, I already prevent EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS unconditionally in efi_check_setvariable() since its deprecated. So I'll keep this as is Cheers /Ilias
On 3/29/24 13:25, Mark Kettenis wrote: >> From: Ilias Apalodimas <ilias.apalodimas@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. Ilias has also been considering a Linux driver for file based variables inside the OS but then you wouldn't need this patch. > >> 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? Best regards Heinrich > > >> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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 >> >>
On Fri, 29 Mar 2024 at 14:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 3/29/24 13:25, Mark Kettenis wrote: > >> From: Ilias Apalodimas <ilias.apalodimas@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.apalodimas@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 > >> > >> >
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,
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. 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. Signed-off-by: Ilias Apalodimas <ilias.apalodimas@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(-) -- 2.37.2