On Mon, 8 Apr 2024 at 11:29, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
>
> On 4/8/24 10:12, Ilias Apalodimas wrote:
> > Hi Heinrich
> >
> > On Mon, 8 Apr 2024 at 09:41, Heinrich Schuchardt <xypron.g...@gmx.de> wrote:
> >>
> >> On 4/6/24 16:01, Ilias Apalodimas wrote:
> >>> Previous patches enabled SetVariableRT using a RAM backend.
> >>> Although EBBR [0] defines a variable format we can teach userspace tools
> >>> and write the altered variables, it's better if we skip the ABI
> >>> requirements completely.
> >>>
> >>> So let's add a new variable, in its own namespace called "VarToFile"
> >>> which contains a binary dump of the updated RT, BS and, NV variables.
> >>>
> >>> Some adjustments are needed to do that. Currently we discard BS-only
> >>> variables in EBS(). We need to preserve those on the OS RAM backend
> >>> that exposes the variables. Since BS-only variables can't appear at RT
> >>> we need to move the memory masking checks from efi_var_collect() to
> >>> efi_get_next_variable_name_mem()/efi_get_variable_mem() and do the
> >>> filtering at runtime. We also need to make efi_var_collect() available
> >>> at runtime, in order to construct the "VarToFile" buffer with BS, RT &
> >>> NV variables.
> >>>
> >>> All users and applications (for linux) have to do when updating a variable
> >>> is dd that variable in the file described by "RTStorageVolatile".
> >>>
> >>> Linux efivarfs uses a first 4 bytes of the output to represent attributes
> >>> in little-endian format. So, storing variables works like this:
> >>>
> >>> $~ efibootmgr -n 0001
> >>> $~ dd
> >>> if=/sys/firmware/efi/efivars/VarToFile-b2ac5fc9-92b7-4acd-aeac-11e818c3130c
> >>> of=/boot/efi/ubootefi.var skip=4 bs=1
> >>>
> >>> [0]
> >>> https://arm-software.github.io/ebbr/index.html#document-chapter5-variable-storage
> >>
> >> This change actually doubles the amount of memory needed to store a
> >> variable at runtime. How do you reflect this in QueryVariableInfo()?
> >>
> >
> > It doesn't double it. Only BS, RT, NV variables are added to VarToFile.
> > QueryVariableInfo isn't supported at runtime, but with the current
> > code we have at boot time the VarToFile would be included in the
> > calculations.
>
> VarToFile does not exist at boot time. Nothing stops the user from
> writing NV variables to fill up the memory buffer. VarToFile will be as
> large as the sum of NV variables and will not fit into the buffer when
> efi_variables_boot_exit_notify() is invoked.
Yes, tbh when I was initially writing this, I was allocating 2x the
variable size buffer to get away from this limitation. But I
eventually decided variables for embedded would be small enough to not
suffer from this that much.
Anyway, I can either allocate 2x size and adjust some code size checks
or move the variable filling in GetVariable as you suggested earlier.
Any preference?
Thanks
/Ilias
>
> >
> >>>
> >>> Suggested-by:Ard Biesheuvel <a...@kernel.org> # dumping all variables to
> >>> a variable
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodi...@linaro.org>
> >>> ---
> >>> include/efi_variable.h | 15 +++-
> >>> lib/efi_loader/efi_boottime.c | 2 +
> >>> lib/efi_loader/efi_var_common.c | 43 +++++------
> >>> lib/efi_loader/efi_var_file.c | 1 -
> >>> lib/efi_loader/efi_var_mem.c | 90 ++++++++++-------------
> >>> lib/efi_loader/efi_variable.c | 118 ++++++++++++++++++++++++------
> >
> > [...]
> >
> >>> * @data: buffer to which the variable value is copied
> >>> * @timep: authentication time (seconds since start of epoch)
> >>> + * @mask: bitmask with required attributes of variables to be
> >>> collected.
> >>> + * variables are only collected if all of the
> >>> required
> >>
> >> This line looks incomplete.
> >
> > Yes, fixed
> >
> >>
> >>> * Return: status code
> >>> */
> >>> efi_status_t __efi_runtime
> >>> efi_get_variable_mem(const u16 *variable_name, const efi_guid_t
> >>> *vendor,
> >>> u32 *attributes, efi_uintn_t *data_size, void *data,
> >>> - u64 *timep);
> >>> + u64 *timep, u32 mask);
> >>>
> >>> /**
> >
> > [...]
> >
> >>> return EFI_UNSUPPORTED;
> >>> @@ -557,11 +593,11 @@ void efi_variables_boot_exit_notify(void)
> >>> const efi_guid_t efi_guid_efi_rt_var_file =
> >>> U_BOOT_EFI_RT_VAR_FILE_GUID;
> >>> const efi_guid_t rt_prop_guid = EFI_RT_PROPERTIES_TABLE_GUID;
> >>> efi_status_t ret;
> >>> + struct efi_var_file *buf;
> >>> + loff_t len;
> >>> + bool fail = false;
> >>>
> >>> if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> >>> - struct efi_rt_properties_table *rt_prop =
> >>> - efi_get_configuration_table(&rt_prop_guid);
> >>> -
> >>> ret = efi_set_variable_int(u"RTStorageVolatile",
> >>> &efi_guid_efi_rt_var_file,
> >>> EFI_VARIABLE_BOOTSERVICE_ACCESS
> >>> |
> >>> @@ -569,11 +605,47 @@ void efi_variables_boot_exit_notify(void)
> >>> EFI_VARIABLE_READ_ONLY,
> >>> sizeof(EFI_VAR_FILE_NAME),
> >>> EFI_VAR_FILE_NAME, false);
> >>> + if (ret != EFI_SUCCESS) {
> >>> + fail = true;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + ret = efi_var_collect(&buf, &len,
> >>> EFI_VARIABLE_NON_VOLATILE);
> >>> + if (ret != EFI_SUCCESS) {
> >>> + fail = true;
> >>> + goto out;
> >>> + }
> >>> +
> >>> + ret = efi_set_variable_int(u"VarToFile",
> >>> + &efi_guid_efi_rt_var_file,
> >>> + EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS,
> >>> + len,
> >>> + buf, false);
> >>> if (ret != EFI_SUCCESS)
> >>> - rt_prop->runtime_services_supported |=
> >>> ~EFI_RT_SUPPORTED_SET_VARIABLE;
> >>> - else
> >>> - log_err("Can't RTStorage. SetVariableRT won't be
> >>> available\n");
> >>> + fail = true;
> >>> +out:
> >>> + if (fail) {
> >>> + efi_set_variable_int(u"RTStorageVolatile",
> >>> + &efi_guid_efi_rt_var_file,
> >>> +
> >>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS |
> >>> + EFI_VARIABLE_READ_ONLY, 0, 0,
> >>> + false);
> >>
> >> Not providing VarToFile because the memory buffer is more than half
> >> filled before ExitBootServices() is unexpected behavior. This needs rework.
> >
> > Why? We should add the SetVariable at runtime to the documentation.
>
> Depending on the number of variables existing at boot time support you
> will be creating VarToFile or not. I can't see that users will fancy this.
>
> Best regards
>
> Heinrich
>
> >
> >>
> >> It is not necessary to create VarToFile in the internal memory buffer.
> >> You could generate it when the user calls GetVariable() in the user
> >> provided buffer.
> >
> > Yes, but that would make some functions look a bit messier, since we
> > have to inject code specifically looking for VarToFile.
> > OTOH it would make SetVariable at runtime easier to read since all the
> > size calculations and variable creation would go away.
> > But in any case, we have to add the variable at runtime (perhaps with
> > a value of 0?) -- users need to know it's there to be able to read it.
> > But if you are fine with the special conditions in GetVariable at
> > runtime, I can easily move the logic to GetVariable.
> >
> > Thanks
> > /Ilias
> >
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>> + efi_set_variable_int(u"VarToFile",
> >>> + &efi_guid_efi_rt_var_file,
> >>> +
> >>> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>> + EFI_VARIABLE_RUNTIME_ACCESS,
> >>> 0, 0,
> >>> + false);
> >>> + } else {
> >>> + struct efi_rt_properties_table *rt_prop =
> >>> + efi_get_configuration_table(&rt_prop_guid);
> >>> +
> >>> + rt_prop->runtime_services_supported |=
> >>> + EFI_RT_SUPPORTED_SET_VARIABLE;
> >>> + }
> >>> }
> >>> +
> >>> /* Switch variable services functions to runtime version */
> >>> efi_runtime_services.get_variable = efi_get_variable_runtime;
> >>> efi_runtime_services.get_next_variable_name =
> >>> diff --git a/lib/efi_loader/efi_variable_tee.c
> >>> b/lib/efi_loader/efi_variable_tee.c
> >>> index dde135fd9f81..9d0e270591ea 100644
> >>> --- a/lib/efi_loader/efi_variable_tee.c
> >>> +++ b/lib/efi_loader/efi_variable_tee.c
> >>> @@ -969,7 +969,6 @@ void efi_variables_boot_exit_notify(void)
> >>> log_err("Can't populate EFI variables. No runtime
> >>> variables will be available\n");
> >>> else
> >>> efi_var_buf_update(var_buf);
> >>> - free(var_buf);
> >>>
> >>> /* Update runtime service table */
> >>> efi_runtime_services.query_variable_info =
> >>> --
> >>> 2.37.2
> >>>
> >>
>
