On 3/18/24 03:46, kojima.masahisa@socionext.com wrote:
Hi Heinrich,
-----Original Message----- From: Heinrich Schuchardt xypron.glpk@gmx.de Sent: Friday, March 15, 2024 4:58 PM To: Kojima, Masahisa/小島 雅久 kojima.masahisa@socionext.com Cc: u-boot@lists.denx.de; Ilias Apalodimas ilias.apalodimas@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@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.
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@ca... is fine for you. Could you, please, add your review.
Best regards
Heinrich
[1] https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Universal/Variabl...
Thanks, Masahisa Kojima
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