[PATCH] efi_loader: accept append write with valid size and data
kojima.masahisa at socionext.com
kojima.masahisa at socionext.com
Fri Mar 15 01:09:48 CET 2024
Hi Ilias,
> -----Original Message-----
> From: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Sent: Thursday, March 14, 2024 10:54 PM
> To: Kojima, Masahisa/小島 雅久 <kojima.masahisa at socionext.com>
> Cc: u-boot at lists.denx.de; Heinrich Schuchardt <xypron.glpk at gmx.de>
> Subject: Re: [PATCH] efi_loader: accept append write with valid size and data
>
> Hi Kojima-san
>
> Apologies for the late reply
>
> On Mon, 4 Mar 2024 at 08:10, Masahisa Kojima
> <kojima.masahisa at socionext.com> wrote:
> >
> > Current "variables" efi_selftest result is inconsistent
> > between the U-Boot file storage and the tee-based StandaloneMM
> > RPMB secure storage.
> > U-Boot file storage implementation does not accept SetVariale
> > call to non-existent variable with EFI_VARIABLE_APPEND_WRITE
> > attribute and valid size and data, however it is accepted
> > in EDK II StandaloneMM implementation.
>
> Yes,
>
> >
> > Since UEFI specification does not clearly describe the behavior
> > of the append write to non-existent variable, let's update
> > the U-Boot file storage implementation to get aligned with
> > the EDK II reference implementation.
> >
> > Signed-off-by: Masahisa Kojima <kojima.masahisa at socionext.com>
> > ---
> > lib/efi_loader/efi_variable.c | 13 +++++-----
> > lib/efi_selftest/efi_selftest_variables.c | 30
> +++++++++++++++++++++--
> > 2 files changed, 35 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index 40f7a0fb10..1693c3387b 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -282,11 +282,8 @@ efi_status_t efi_set_variable_int(const u16
> *variable_name,
> > }
> > time = var->time;
> > } else {
> > - if (delete || append)
> > - /*
> > - * Trying to delete or to update a non-existent
> > - * variable.
> > - */
> > + if (delete)
> > + /* Trying to delete a non-existent variable. */
> > return EFI_NOT_FOUND;
>
> I am not sure about this tbh. The UEFI spec says
> "When the EFI_VARIABLE_APPEND_WRITE attribute is set, then a
> SetVariable() call with a DataSize of zero will not cause any change
> to the variable value (the timestamp
> associated with the variable may be updated however, even if no new
> data value is provided;see the description
> of the EFI_VARIABLE_AUTHENTICATION_2 descriptor below). In this case
> the DataSize will not be zero
> since the EFI_VARIABLE_AUTHENTICATION_2 descriptor will be populated)"
>
> I think this assumes the variable exists. Is there a different chapter
> in the spec that describes the behavior?
No, I could not find it.
I'm also not sure what the UEFI spec expects.
>
> > }
> >
> > @@ -329,7 +326,11 @@ efi_status_t efi_set_variable_int(const u16
> *variable_name,
> > /* EFI_NOT_FOUND has been handled before */
> > attributes = var->attr;
> > ret = EFI_SUCCESS;
> > - } else if (append) {
> > + } else if (append && var) {
> > + /*
> > + * data is appended if EFI_VARIABLE_APPEND_WRITE is
> set and
> > + * variable exists.
> > + */
> > u16 *old_data = var->name;
> >
> > for (; *old_data; ++old_data)
> > diff --git a/lib/efi_selftest/efi_selftest_variables.c
> b/lib/efi_selftest/efi_selftest_variables.c
> > index c7a3fdbaa6..0c2c76599e 100644
> > --- a/lib/efi_selftest/efi_selftest_variables.c
> > +++ b/lib/efi_selftest/efi_selftest_variables.c
> > @@ -131,13 +131,39 @@ static int execute(void)
> > (unsigned int)len);
> > if (memcmp(data, v, len))
> > efi_st_todo("GetVariable returned wrong value\n");
> > - /* Append variable 2 */
> > + /* Append variable 2, write to non-existent variable */
> > ret = runtime->set_variable(u"efi_none", &guid_vendor1,
> >
> EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > EFI_VARIABLE_APPEND_WRITE,
> > 15, v);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable(APPEND_WRITE) with valid size
> and data to non-existent variable must be succcessful\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + len = EFI_ST_MAX_DATA_SIZE;
> > + ret = runtime->get_variable(u"efi_none", &guid_vendor1,
> > + &attr, &len, data);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("GetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + if (len != 15)
> > + efi_st_todo("GetVariable returned wrong length %u\n",
> > + (unsigned int)len);
> > + if (memcmp(data, v, len))
> > + efi_st_todo("GetVariable returned wrong value\n");
> > + /* Delete variable efi_none */
> > + ret = runtime->set_variable(u"efi_none", &guid_vendor1,
> > + 0, 0, NULL);
> > + if (ret != EFI_SUCCESS) {
> > + efi_st_error("SetVariable failed\n");
> > + return EFI_ST_FAILURE;
> > + }
> > + len = EFI_ST_MAX_DATA_SIZE;
> > + ret = runtime->get_variable(u"efi_none", &guid_vendor1,
> > + &attr, &len, data);
> > if (ret != EFI_NOT_FOUND) {
> > - efi_st_error("SetVariable(APPEND_WRITE) with size 0 to
> non-existent variable returns wrong code\n");
> > + efi_st_error("Variable was not deleted\n");
> > return EFI_ST_FAILURE;
>
> This used to fail as well [0]. Does it work now?
Yes. With this patch applied, both U-Boot file storage and StMM
pass the efi selftest.
Thanks,
Masahisa Kojima
>
> > }
> > /* Enumerate variables */
> > --
> > 2.34.1
> >
>
> [0]
> https://lore.kernel.org/u-boot/CAC_iWjL2G2mvQb6WNGLTxr+xj64xrYuOKr8G
> 3-3z8Lv7yX5XSg at mail.gmail.com/
>
> Regards
> /Ilias
More information about the U-Boot
mailing list