[PATCH v1 1/4] efi_loader: conditionally enable SetvariableRT
Heinrich Schuchardt
xypron.glpk at gmx.de
Mon Apr 8 08:15:21 CEST 2024
On 4/6/24 16:01, Ilias Apalodimas wrote:
> When EFI variables are stored on file we don't allow SetVariableRT,
Neither in the UEFI standard nor in U-Boot we have a function SetVariableRT.
%s/SetVariableRT/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 GetVariableRT we copy runtime variables in RAM and expose them to
%s/GetVariabeRT/GetVariable at runtime/
> 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
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> ---
> lib/efi_loader/Kconfig | 16 +++
> lib/efi_loader/efi_runtime.c | 5 +
> lib/efi_loader/efi_variable.c | 114 ++++++++++++++++--
> .../efi_selftest_variables_runtime.c | 13 +-
> 4 files changed, 135 insertions(+), 13 deletions(-)
>
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index a7c3e05c13a0..b210ceea6d64 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -63,6 +63,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 18da6892e796..8ebbea7e5c69 100644
> --- a/lib/efi_loader/efi_runtime.c
> +++ b/lib/efi_loader/efi_runtime.c
> @@ -10,6 +10,7 @@
> #include <dm.h>
> #include <elf.h>
> #include <efi_loader.h>
> +#include <efi_variable.h>
> #include <log.h>
> #include <malloc.h>
> #include <rtc.h>
> @@ -126,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 6fe3792a12a5..f79041e6bedd 100644
> --- a/lib/efi_loader/efi_variable.c
> +++ b/lib/efi_loader/efi_variable.c
> @@ -218,17 +218,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)
Can we reuse this function in efi_query_variable_info_int()?
> {
> - 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;
>
> @@ -260,6 +263,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);
> @@ -452,6 +474,78 @@ 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)) {
This line should be indented by one tab.
Can we put the EFI_UNSUPPORTED exit on top to avoid a very long if branch:
if (!IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
return EFI_UNSUPPORTED.
If we implement SetVariable() at runtime, we should also enable
QueryVariableInfo().
> + struct efi_var_entry *var;
> + efi_uintn_t ret;
> + bool append, delete;
> + u64 time = 0;
> +
> + /*
> + * Authenticated variables are not supported the rest of the checks
> + * are in setvariable_allowed()
Please, describe here why they are not implemented, yet.
> + */
> + 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;
> +
Isn't this duplicating logic from efi_set_variable_int()?
Can we carve out more common code?
Best regards
Heinrich
> + 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 = 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;
> }
>
> 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) {
> + 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,
More information about the U-Boot
mailing list