[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