[PATCH v2 1/4] efi_loader: conditionally enable SetvariableRT
Ilias Apalodimas
ilias.apalodimas at linaro.org
Wed Apr 17 14:33:40 CEST 2024
On Wed, 17 Apr 2024 at 15:28, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> On 17.04.24 12:19, Ilias Apalodimas wrote:
> > When we store EFI variables on file we don't allow SetVariable at runtime,
> > since the OS doesn't know how to access or write that file. At the same
> > time keeping the U-Boot drivers alive in runtime sections and performing
> > writes from the firmware is dangerous -- if at all possible.
> >
> > For GetVariable at runtime we copy runtime variables in RAM and expose them
> > to the OS. Add a Kconfig option and provide SetVariable at runtime using
> > the same memory backend. The OS will be responsible for syncing the RAM
> > contents to the file, otherwise any changes made during runtime won't
> > persist reboots.
> >
> > It's worth noting that the variable store format is defined in EBBR [0]
> > and authenticated variables are explicitly prohibited, since they have
> > to be stored on a medium that's tamper and rollback protected.
> >
> > - pre-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (ro,nosuid,nodev,noexec,relatime)
> >
> > $~ efibootmgr -n 0001
> > Could not set BootNext: Read-only file system
> >
> > - post-patch
> > $~ mount | grep efiva
> > efivarfs on /sys/firmware/efi/efivars type efivarfs (rw,nosuid,nodev,noexec,relatime)
> >
> > $~ efibootmgr -n 0001
> > BootNext: 0001
> > BootCurrent: 0000
> > BootOrder: 0000,0001
> > Boot0000* debian HD(1,GPT,bdae5610-3331-4e4d-9466-acb5caf0b4a6,0x800,0x100000)/File(EFI\debian\grubaa64.efi)
> > Boot0001* virtio 0 VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,0000000000000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,850000001f000000)/VenHw(e61d73b9-a384-4acc-aeab-82e828f3628b,1600850000000000){auto_created_boot_option}
> >
> > $~ efivar -p -n 8be4df61-93ca-11d2-aa0d-00e098032b8c-BootNext
> > GUID: 8be4df61-93ca-11d2-aa0d-00e098032b8c
> > Name: "BootNext"
> > Attributes:
> > Non-Volatile
> > Boot Service Access
> > Runtime Service Access
> > Value:
> > 00000000 01 00
> >
> > [0] https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> > lib/efi_loader/Kconfig | 16 +++
> > lib/efi_loader/efi_runtime.c | 4 +
> > lib/efi_loader/efi_variable.c | 115 ++++++++++++++++--
> > .../efi_selftest_variables_runtime.c | 13 +-
> > 4 files changed, 134 insertions(+), 14 deletions(-)
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index e13a6f9f4c3a..cc8371a3bb4c 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -62,6 +62,22 @@ config EFI_VARIABLE_FILE_STORE
> > Select this option if you want non-volatile UEFI variables to be
> > stored as file /ubootefi.var on the EFI system partition.
> >
> > +config EFI_RT_VOLATILE_STORE
> > + bool "Allow variable runtime services in volatile storage (e.g RAM)"
> > + depends on EFI_VARIABLE_FILE_STORE
> > + help
> > + When EFI variables are stored on file we don't allow SetVariableRT,
> > + since the OS doesn't know how to write that file. At he same time
> > + we copy runtime variables in DRAM and support GetVariableRT
> > +
> > + Enable this option to allow SetVariableRT on the RAM backend of
> > + the EFI variable storage. The OS will be responsible for syncing
> > + the RAM contents to the file, otherwise any changes made during
> > + runtime won't persist reboots.
> > + Authenticated variables are not supported. Note that this will
> > + violate the EFI spec since writing auth variables will return
> > + EFI_INVALID_PARAMETER
> > +
> > config EFI_MM_COMM_TEE
> > bool "UEFI variables storage service via the trusted world"
> > depends on OPTEE
> > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> > index a61c9a77b13f..dde083b09665 100644
> > --- a/lib/efi_loader/efi_runtime.c
> > +++ b/lib/efi_loader/efi_runtime.c
> > @@ -127,6 +127,10 @@ efi_status_t efi_init_runtime_supported(void)
> > EFI_RT_SUPPORTED_SET_VIRTUAL_ADDRESS_MAP |
> > EFI_RT_SUPPORTED_CONVERT_POINTER;
> >
> > + if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
> > + rt_table->runtime_services_supported |=
> > + EFI_RT_SUPPORTED_SET_VARIABLE;
> > +
> > /*
> > * This value must be synced with efi_runtime_detach_list
> > * as well as efi_runtime_services.
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index e6c1219a11c8..abc2a3402f42 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -219,17 +219,20 @@ efi_get_next_variable_name_int(efi_uintn_t *variable_name_size,
> > return efi_get_next_variable_name_mem(variable_name_size, variable_name, vendor);
> > }
> >
> > -efi_status_t efi_set_variable_int(const u16 *variable_name,
> > - const efi_guid_t *vendor,
> > - u32 attributes, efi_uintn_t data_size,
> > - const void *data, bool ro_check)
> > +/**
> > + * setvariable_allowed() - checks defined by the UEFI spec for setvariable
> > + *
> > + * @variable_name: name of the variable
> > + * @vendor: vendor GUID
> > + * @attributes: attributes of the variable
> > + * @data_size: size of the buffer with the variable value
> > + * @data: buffer with the variable value
> > + * Return: status code
> > + */
> > +static efi_status_t __efi_runtime
> > +setvariable_allowed(const u16 *variable_name, const efi_guid_t *vendor,
> > + u32 attributes, efi_uintn_t data_size, const void *data)
> > {
> > - struct efi_var_entry *var;
> > - efi_uintn_t ret;
> > - bool append, delete;
> > - u64 time = 0;
> > - enum efi_auth_var_type var_type;
> > -
> > if (!variable_name || !*variable_name || !vendor)
> > return EFI_INVALID_PARAMETER;
> >
> > @@ -261,6 +264,25 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
> > !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)))
> > return EFI_INVALID_PARAMETER;
> >
> > + return EFI_SUCCESS;
> > +}
> > +
> > +efi_status_t efi_set_variable_int(const u16 *variable_name,
> > + const efi_guid_t *vendor,
> > + u32 attributes, efi_uintn_t data_size,
> > + const void *data, bool ro_check)
> > +{
> > + struct efi_var_entry *var;
> > + efi_uintn_t ret;
> > + bool append, delete;
> > + u64 time = 0;
> > + enum efi_auth_var_type var_type;
> > +
> > + ret = setvariable_allowed(variable_name, vendor, attributes, data_size,
> > + data);
> > + if (ret != EFI_SUCCESS)
> > + return ret;
> > +
> > /* check if a variable exists */
> > var = efi_var_mem_find(vendor, variable_name, NULL);
> > append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> > @@ -454,7 +476,78 @@ efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *vendor,
> > u32 attributes, efi_uintn_t data_size,
> > const void *data)
> > {
> > - return EFI_UNSUPPORTED;
> > + struct efi_var_entry *var;
> > + efi_uintn_t ret;
> > + bool append, delete;
> > + u64 time = 0;
> > +
> > + if (!IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE))
> > + return EFI_UNSUPPORTED;
> > +
> > + /*
> > + * Authenticated variables are not supported. The EFI spec
> > + * in §32.3.6 requires keys to be stored in non-volatile storage which
> > + * is tamper and delete resistant.
> > + * The rest of the checks are in setvariable_allowed()
> > + */
> > + if (attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS)
> > + return EFI_INVALID_PARAMETER;
> > + /* BS only variables are hidden deny writing them */
> > + if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
> > + return EFI_INVALID_PARAMETER;
> > +
> > + ret = setvariable_allowed(variable_name, vendor, attributes, data_size,
> > + data);
> > + if (ret != EFI_SUCCESS)
> > + return ret;
> > +
> > + /* check if a variable exists */
> > + var = efi_var_mem_find(vendor, variable_name, NULL);
> > + append = !!(attributes & EFI_VARIABLE_APPEND_WRITE);
> > + attributes &= ~EFI_VARIABLE_APPEND_WRITE;
> > + delete = !append && (!data_size || !attributes);
> > +
> > + if (var) {
> > + if (var->attr & EFI_VARIABLE_READ_ONLY ||
> > + !(var->attr & EFI_VARIABLE_NON_VOLATILE))
> > + return EFI_WRITE_PROTECTED;
> > +
> > + /* attributes won't be changed */
> > + if (!delete && (((var->attr & ~EFI_VARIABLE_READ_ONLY) !=
> > + (attributes & ~EFI_VARIABLE_READ_ONLY))))
> > + return EFI_INVALID_PARAMETER;
> > + time = var->time;
> > + } else {
> > + if (!(attributes & EFI_VARIABLE_NON_VOLATILE))
> > + return EFI_INVALID_PARAMETER;
> > + if (append && !data_size)
> > + return EFI_SUCCESS;
> > + if (delete)
> > + return EFI_NOT_FOUND;
> > + }
> > +
> > + if (delete) {
> > + /* EFI_NOT_FOUND has been handled before */
> > + attributes = var->attr;
> > + ret = EFI_SUCCESS;
> > + } else if (append && var) {
> > + u16 *old_data = (void *)((uintptr_t)var->name +
> > + sizeof(u16) * (u16_strlen(var->name) + 1));
> > +
> > + ret = efi_var_mem_ins(variable_name, vendor, attributes,
> > + var->length, old_data, data_size, data,
> > + time);
> > + } else {
> > + ret = efi_var_mem_ins(variable_name, vendor, attributes,
> > + data_size, data, 0, NULL, time);
> > + }
> > +
> > + if (ret != EFI_SUCCESS)
> > + return ret;
> > + /* We are always inserting new variables, get rid of the old copy */
> > + efi_var_mem_del(var);
> > +
> > + return EFI_SUCCESS;
> > }
> >
> > /**
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > index 4700d9424105..4c9405c0a7c7 100644
> > --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> > +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -62,9 +62,16 @@ static int execute(void)
> > EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > EFI_VARIABLE_RUNTIME_ACCESS,
> > 3, v + 4);
> > - if (ret != EFI_UNSUPPORTED) {
> > - efi_st_error("SetVariable failed\n");
> > - return EFI_ST_FAILURE;
> > + if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > + if (ret != EFI_INVALID_PARAMETER) {
>
> A comment might be helpful here:
>
> /* At runtime only non-volatile variables may be set. */
>
> Otherwise looks good to me.
>
> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
Thanks, Heinrich!
I'll add the comment in v3
>
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + } else {
> > + if (ret != EFI_UNSUPPORTED) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > }
> > len = EFI_ST_MAX_DATA_SIZE;
> > ret = runtime->get_variable(u"PlatformLangCodes", &guid_vendor0,
>
More information about the U-Boot
mailing list