[PATCH] efi_loader: accept append write with valid size and data
kojima.masahisa at socionext.com
kojima.masahisa at socionext.com
Thu Mar 21 07:42:04 CET 2024
> -----Original Message-----
> From: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Sent: Thursday, March 21, 2024 3:21 PM
> To: Kojima, Masahisa/小島 雅久 <kojima.masahisa at socionext.com>
> Cc: u-boot at lists.denx.de; ilias.apalodimas at linaro.org
> Subject: Re: [PATCH] efi_loader: accept append write with valid size and data
>
> On 3/18/24 03:46, kojima.masahisa at socionext.com wrote:
> > 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 behavior if needed.
> > # I'm not sure that the edk2 implementation of #4 is correct and expected
> anyway.
>
> Hello Kojima,
>
> I don't believe EDK II will change. So let us return EFI_SUCCESS in case
> #4, too.
>
> Could you, please, re-submit the patch with that change.
OK, I will re-submit the patch modifies both #3 and #4 cases.
>
> I hope
> [PATCH 1/1] .mailmap entry for Masahisa Kojima
> https://lore.kernel.org/u-boot/20240318104718.18365-1-heinrich.schuchardt
> @canonical.com/
> is fine for you. Could you, please, add your review.
Thank you, I will do.
Best Regards,
Masahisa Kojima
>
> Best regards
>
> Heinrich
>
> >
> > [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