On 3/15/24 08:03, Ilias Apalodimas wrote:
Hi Kojima-san
On Fri, 15 Mar 2024 at 02:10, kojima.masahisa@socionext.com wrote:
Hi Ilias,
-----Original Message----- From: Ilias Apalodimas ilias.apalodimas@linaro.org Sent: Thursday, March 14, 2024 10:54 PM To: Kojima, Masahisa/小島 雅久 kojima.masahisa@socionext.com Cc: u-boot@lists.denx.de; Heinrich Schuchardt xypron.glpk@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@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@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@mail.gmail.com/
Regards /Ilias