[PATCH v2 1/3] efi_loader: enable QueryVariableInfo at runtime for file backed variables

Ilias Apalodimas ilias.apalodimas at linaro.org
Thu Apr 25 12:25:29 CEST 2024


Hi Heinrich,

On Thu, 25 Apr 2024 at 11:32, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
> On 25.04.24 07:18, Ilias Apalodimas wrote:
> > Since commit c28d32f946f0 ("efi_loader: conditionally enable SetvariableRT")
> > we are enabling the last bits of missing runtime services.
> > Add support for QueryVariableInfo which we already support at boottime
> > and we just need to mark some fucntions available at runtime and move
> > some checks around.
> >
> > It's worth noting that pointer checks for maxmimum and remaining
> > variable storage aren't when we store variables on the RPMB, since the
> > Secure World backend is already performing them.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > ---
> > Changes since v1:
> > - require EFI_VARIABLE_RUNTIME_ACCESS to be set at runtime
> > - return EFI_UNSUPPORTED for auth variables
> >   lib/efi_loader/efi_runtime.c                  |  4 +++
> >   lib/efi_loader/efi_var_common.c               |  6 -----
> >   lib/efi_loader/efi_variable.c                 | 25 ++++++++++++++-----
> >   lib/efi_loader/efi_variable_tee.c             |  5 ++++
> >   .../efi_selftest_variables_runtime.c          | 14 ++++++++---
> >   5 files changed, 39 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_runtime.c b/lib/efi_loader/efi_runtime.c
> > index 73831c527e00..011bcd04836d 100644
> > --- a/lib/efi_loader/efi_runtime.c
> > +++ b/lib/efi_loader/efi_runtime.c
> > @@ -129,6 +129,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_VARIABLE_FILE_STORE))
> > +             rt_table->runtime_services_supported |=
> > +                     EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO;
> > +
> >       if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> >               u8 s = 0;
> >
> > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > index 961139f005af..ea8d2a4cf98c 100644
> > --- a/lib/efi_loader/efi_var_common.c
> > +++ b/lib/efi_loader/efi_var_common.c
> > @@ -1,4 +1,3 @@
> > -// SPDX-License-Identifier: GPL-2.0+
> >   /*
> >    * UEFI runtime variable services
> >    *
> > @@ -163,11 +162,6 @@ efi_status_t EFIAPI efi_query_variable_info(
> >       EFI_ENTRY("%x %p %p %p", attributes, maximum_variable_storage_size,
> >                 remaining_variable_storage_size, maximum_variable_size);
> >
> > -     if (!maximum_variable_storage_size ||
> > -         !remaining_variable_storage_size ||
> > -         !maximum_variable_size)
> > -             return EFI_EXIT(EFI_INVALID_PARAMETER);
> > -
> >       ret = efi_query_variable_info_int(attributes,
> >                                         maximum_variable_storage_size,
> >                                         remaining_variable_storage_size,
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 0cbed53d1dbf..1cc02acb3b26 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -406,12 +406,15 @@ efi_status_t efi_set_variable_int(const u16 *variable_name,
> >       return EFI_SUCCESS;
> >   }
> >
> > -efi_status_t efi_query_variable_info_int(u32 attributes,
> > -                                      u64 *maximum_variable_storage_size,
> > -                                      u64 *remaining_variable_storage_size,
> > -                                      u64 *maximum_variable_size)
> > +efi_status_t __efi_runtime
> > +efi_query_variable_info_int(u32 attributes,
> > +                         u64 *maximum_variable_storage_size,
> > +                         u64 *remaining_variable_storage_size,
> > +                         u64 *maximum_variable_size)
> >   {
> > -     if (attributes == 0)
> > +     if (!maximum_variable_storage_size ||
> > +         !remaining_variable_storage_size ||
> > +         !maximum_variable_size || !attributes)
> >               return EFI_INVALID_PARAMETER;
> >
> >       /* EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS is deprecated */
> > @@ -460,7 +463,17 @@ static efi_status_t __efi_runtime EFIAPI efi_query_variable_info_runtime(
> >                       u64 *remaining_variable_storage_size,
> >                       u64 *maximum_variable_size)
> >   {
> > -     return EFI_UNSUPPORTED;
>
> According to the UEFI 2.10 specification EFI_UNSUPPORTED must be returned if
>
> "The attribute is not supported on this platform, and the
> MaximumVariableStorageSize, RemainingVariableStorageSize,
> MaximumVariableSize are undefined."
>
> I wonder if in the spec the 'and' should be replaced by 'or'.

Probably 'or'

>
> I guess we should return EFI_UNSUPPORTED if
> CONFIG_EFI_VARIABLE_RUNTIME_ACCESS=n and change the corresponding test
> accordingly. This check should be placed first.

The spec isn't clear indeed, but looking at the flags usually passed
in QueyVariable, there's nothing that checks for SetVariable at
runtime -- which isn't even an attr, it's a property of the RT_PROP
table.
We do return EFI_UNSUPPORTED for authenticated variables, which we
can't reliably support (and auth variables have their own attr).
I would prefer merging this with the existing checks and in the future
(perhaps the next stable release) make CONFIG_EFI_RT_VOLATILE_STORE=y
by default or get rid of the config options entirely once the
userspace tools are mature enough to support it.

In any case, I don't have a strong opinion, I am fine with both. Just
tell me what you prefer.

Regards
/Ilias

>
> Best regards
>
> Heinrich
>
> > +     if (!(attributes & EFI_VARIABLE_RUNTIME_ACCESS))
> > +             return EFI_INVALID_PARAMETER;
> > +     if ((attributes & (EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS |
> > +                        EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS |
> > +                        EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS)))
> > +             return EFI_UNSUPPORTED;
> > +
> > +     return efi_query_variable_info_int(attributes,
> > +                                        maximum_variable_storage_size,
> > +                                        remaining_variable_storage_size,
> > +                                        maximum_variable_size);
> >   }
> >
> >   /**
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > index 4f1aa298da13..8b6b0a390869 100644
> > --- a/lib/efi_loader/efi_variable_tee.c
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -873,6 +873,11 @@ efi_status_t efi_query_variable_info_int(u32 attributes,
> >       efi_status_t ret;
> >       u8 *comm_buf;
> >
> > +     if (!max_variable_storage_size ||
> > +         !remain_variable_storage_size ||
> > +         !max_variable_size || !attributes)
> > +             return EFI_INVALID_PARAMETER;
> > +
> >       payload_size = sizeof(*mm_query_info);
> >       comm_buf = setup_mm_hdr((void **)&mm_query_info, payload_size,
> >                               SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
> > diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > index afa91be62c85..5794a7b2d405 100644
> > --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> > +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> > @@ -60,9 +60,17 @@ static int execute(void)
> >       ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> >                                          &max_storage, &rem_storage,
> >                                          &max_size);
> > -     if (ret != EFI_UNSUPPORTED) {
> > -             efi_st_error("QueryVariableInfo failed\n");
> > -             return EFI_ST_FAILURE;
> > +
> > +     if (IS_ENABLED(CONFIG_EFI_VARIABLE_FILE_STORE)) {
> > +             if (ret != EFI_SUCCESS) {
> > +                     efi_st_error("QueryVariableInfo failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +     } else {
> > +             if (ret != EFI_UNSUPPORTED) {
> > +                     efi_st_error("QueryVariableInfo failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> >       }
> >
> >       ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > --
> > 2.40.1
> >
>


More information about the U-Boot mailing list