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

kojima.masahisa at socionext.com kojima.masahisa at socionext.com
Mon Mar 18 03:46:04 CET 2024


Hi Heinrich,

> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Sent: Friday, March 15, 2024 4:58 PM
> To: Kojima, Masahisa/小島 雅久 <kojima.masahisa at socionext.com>
> Cc: u-boot at lists.denx.de; Ilias Apalodimas <ilias.apalodimas at linaro.org>
> Subject: Re: [PATCH] efi_loader: accept append write with valid size and data
> 
> On 3/15/24 08:03, Ilias Apalodimas wrote:
> > 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...
> 
> Hello Kojima,
> 
> Thank you for pointing out the inconsistencies.
> 
> I have created https://mantis.uefi.org/mantis/view.php?id=2447 to
> discuss the expected behavior.

Thank you.
My company is not the member of uefi and probably I could not create the account.

> 
> The logic in MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c is:
> 
> If the variable does not exist and EFI_VARIABLE_APPEND_WRITE is set and
> the size is non-zero, the variable is created.
> If the variable does not exist and EFI_VARIABLE_APPEND_WRITE is set and
> the size is zero, the variable is not created and EFI_SUCCESS is returned.
> 
> I am not able to derive this from the specification.
> 
> In our selftest we should test the following four cases of
> EFI_VARIABLE_APPEND_WRITE:
> 
> variable existing, size non-zero
> variable existing, size zero
> variable non-existent, size non-zero
> variable non-existent, size zero
> 
> Could you, please, check if the StMM logic matches
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c for all four cases.

The variable service logic is common, StMM also uses
MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c for Variable Service[1].

> 
> If yes, we should change the our file based implementation to match these.

Current situation of append write is as follows.

#1 variable existing, size non-zero => OK
 StMM: append write
 U-Boot: append write

#2 variable existing, size zero => OK
 StMM: will not delete, return EFI_SUCCESS
 U-Boot: will not delete, return EFI_SUCCESS

#3 variable non-existent, size non-zero => inconsistent
 StMM: create new variable
 U-Boot: return EFI_NOT_FOUND

#4 variable non-existent, size zero  => inconsistent
 StMM: will not create variable, return EFI_SUCCESS
 U-Boot: return EFI_NOT_FOUND

My original patch tried to fix above case #3.
Currently U-Boot does not have selftest for case #4, we need to add a test
and also modify the bahavior if needed.
 # I'm not sure that the edk2 implementation of #4 is correct and expected anyway.

[1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c#L94

Thanks,
Masahisa Kojima

> 
> Best regards
> 
> Heinrich
> 
> >>>
> >>>>          }
> >>>>          /* 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