Hi Heinrich,
On Thu, 25 Apr 2024 at 11:32, Heinrich Schuchardt <xypron.g...@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.apalodi...@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
> >
>
