[PATCH] efi_loader: accept append write with valid size and data

[Ilias Apalodimas]

Hi Kojima-san

On Fri, 15 Mar 2024 at 02:10, <kojima.masahisa at socionext.com> wrote:
>
> 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.

I don't have any objections to aligning this with EDK2. But can we add
a comment about it as well?
Heinrich any preference here?

[...]

> > 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.

Yes, that's obviously a hack. The reason we didn't bother fixing it
back then is because stmm is not running in our CI, but I should have
added a comment on that instead of having it on email history...

Thanks
/Ilias
>
> 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