[RFC PATCH] efi_loader: conditionally enable SetvariableRT

[Ilias Apalodimas]

[...]

> >
> > +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
>
> Hello Ilias,
>
> I like the idea of adding this for experimenting. We should express that
> this is just experimental and may be removed in future releases, if we
> find a better solution.

U-Boot documentation is good enough? I've cc'ed Peter (which maintain
efivar AFAIK). If we agree on how to teach efivar/efibootmgr to use
that file, I can describe it in docs

>
> %s/storate/stroage/
>
> > +       the RAM contents to the file, otherwise any changes made during
> > +       runtime won't persist reboots.
> > +       Authenticated variables are not supported.
>
> Should we mention here that this behavior is not compliant with the UEFI
> specification?
>
> Should we unset CONFIG_EFI_EBBR_2_1_CONFORMANCE if this flag is set?

No, I don't think so. I'll ask around to make sure. However, EBBR
already standardizes the file format and explicitly prevents storing
authenticated variables. There's a separate test suite from arm called
SIE (SEcurity interface extensions) which is complementary to
SystemReady-IR and tests EFI authenticated variables.

>
> > +
> >   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)
>
> Please, use 'if' instead of #if. The

Sure

>
> > +     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)
>
> Adding EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS here would make it
> clearer that all authenticated variables are unsupported.

Ok, will do

>
> Non-volatile variables are read-only at runtime. Please, check that
> EFI_VARIABLE_NON_VOLATILE is set.

You mean volatile right? But yea this needs fixing

>
> if ((attributes & (
>    EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS |
>    EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS |
>    EFI_VARIABLE_ENHANCED_AUTHENTICATED_ACCESS)) ||
>    (~attributes & EFI_VARIABLE_NON_VOLATILE)
>      return EFI_INVALID_PARAMETER;
>
> > +                     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;
>
> Why is the u32 conversion necessary? The result should be the same
> without conversion.
>
> The UEFI specification defines the different attribute constants as
> 32bit. Why do we define them as 64bit in include/efi.h?

I guess this is an existing misinterpretation of the spec.
I copy pasted this check from efi_set_variable_int(). It seems we
should fix both.
I'll include the fix in v2

>
> > +     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;
>
> ditto
>
> > +             time = var->time;
> > +     } else {
> > +             if (delete || append)
>
> EDK II allows to create a variable with EFI_VARIABLE_APPEND_WRITE. See
> previous discussions about changing this at boot time.

Yes, the reason I kept it like this is that we didn't merge that yet
and I wanted to keep the same behavior. I'll update this one

[...]

Thanks
/Ilias