[RFC PATCH] efi_loader: conditionally enable SetvariableRT
Ilias Apalodimas
ilias.apalodimas at linaro.org
Fri Mar 29 14:51:20 CET 2024
On Fri, 29 Mar 2024 at 14:57, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 3/29/24 13:25, Mark Kettenis wrote:
> >> From: Ilias Apalodimas <ilias.apalodimas at 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 at 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
> >>
> >>
>
More information about the U-Boot
mailing list