[PATCH] efi_loader: accept append write with valid size and data
Heinrich Schuchardt
xypron.glpk at gmx.de
Thu Mar 21 07:21:19 CET 2024
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.
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.
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