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

[Heinrich Schuchardt]

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.

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.

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

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