On Tue, 2 Apr 2024 at 12:09, Masahisa Kojima <[email protected]> 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, > it return EFI_NOT_FOUND. > However it is accepted and new variable is created in EDK II > StandaloneMM implementation if valid data and size are specified. > If data size is 0, EFI_SUCCESS is returned. > > 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 <[email protected]> > --- > v1 -> v2 > - return EFI_SUCCESS for append write with data size 0 > - add comment that we follow the EDK II implementation > > lib/efi_loader/efi_variable.c | 26 +++++++++--- > lib/efi_selftest/efi_selftest_variables.c | 48 ++++++++++++++++++++++- > 2 files changed, 66 insertions(+), 8 deletions(-) > > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c > index 40f7a0fb10..d1db7ade0a 100644 > --- a/lib/efi_loader/efi_variable.c > +++ b/lib/efi_loader/efi_variable.c > @@ -282,11 +282,21 @@ 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. > - */ > + /* > + * UEFI specification does not clearly describe the expected > + * behavior of append write with data size 0, we follow > + * the EDK II reference implementation. > + */ > + if (append && !data_size) > + return EFI_SUCCESS; > + > + /* > + * EFI_VARIABLE_APPEND_WRITE to non-existent variable is > accepted > + * and new variable is created in EDK II reference > implementation. > + * We follow it and only check the deletion here. > + */ > + if (delete) > + /* Trying to delete a non-existent variable. */ > return EFI_NOT_FOUND; > } > > @@ -329,7 +339,11 @@ efi_status_t efi_set_variable_int(const u16 > *variable_name, > /* EFI_NOT_FOUND has been handled before */ > attributes = var->attr; > ret = EFI_SUCCESS; > - } else if (append) { > + } else if (append && var) { > + /* > + * data is appended if EFI_VARIABLE_APPEND_WRITE is set and > + * variable exists. > + */ > u16 *old_data = var->name; > > for (; *old_data; ++old_data) > diff --git a/lib/efi_selftest/efi_selftest_variables.c > b/lib/efi_selftest/efi_selftest_variables.c > index c7a3fdbaa6..39ad03a090 100644 > --- a/lib/efi_selftest/efi_selftest_variables.c > +++ b/lib/efi_selftest/efi_selftest_variables.c > @@ -131,13 +131,57 @@ static int execute(void) > (unsigned int)len); > if (memcmp(data, v, len)) > efi_st_todo("GetVariable returned wrong value\n"); > - /* Append variable 2 */ > + > + /* Append variable 2, write to non-existent variable with datasize=0 > */ > + ret = runtime->set_variable(u"efi_none", &guid_vendor1, > + EFI_VARIABLE_BOOTSERVICE_ACCESS | > + EFI_VARIABLE_APPEND_WRITE, > + 0, v); > + if (ret != EFI_SUCCESS) { > + efi_st_error( > + "SetVariable(APPEND_WRITE) with size 0 to > non-existent variable returns wrong code\n"); > + return EFI_ST_FAILURE; > + } > + len = EFI_ST_MAX_DATA_SIZE; > + ret = runtime->get_variable(u"efi_none", &guid_vendor1, > + &attr, &len, data); > + if (ret != EFI_NOT_FOUND) { > + efi_st_error("Variable must not be created\n"); > + return EFI_ST_FAILURE; > + } > + /* Append variable 2, write to non-existent variable with valid data > size*/ > ret = runtime->set_variable(u"efi_none", &guid_vendor1, > EFI_VARIABLE_BOOTSERVICE_ACCESS | > EFI_VARIABLE_APPEND_WRITE, > 15, v); > + if (ret != EFI_SUCCESS) { > + efi_st_error("SetVariable(APPEND_WRITE) with valid size and > data to non-existent variable must be succcessful\n"); > + return EFI_ST_FAILURE; > + } > + len = EFI_ST_MAX_DATA_SIZE; > + ret = runtime->get_variable(u"efi_none", &guid_vendor1, > + &attr, &len, data); > + if (ret != EFI_SUCCESS) { > + efi_st_error("GetVariable failed\n"); > + return EFI_ST_FAILURE; > + } > + if (len != 15) > + efi_st_todo("GetVariable returned wrong length %u\n", > + (unsigned int)len); > + if (memcmp(data, v, len)) > + efi_st_todo("GetVariable returned wrong value\n"); > + /* Delete variable efi_none */ > + ret = runtime->set_variable(u"efi_none", &guid_vendor1, > + 0, 0, NULL); > + if (ret != EFI_SUCCESS) { > + efi_st_error("SetVariable failed\n"); > + return EFI_ST_FAILURE; > + } > + len = EFI_ST_MAX_DATA_SIZE; > + ret = runtime->get_variable(u"efi_none", &guid_vendor1, > + &attr, &len, data); > if (ret != EFI_NOT_FOUND) { > - efi_st_error("SetVariable(APPEND_WRITE) with size 0 to > non-existent variable returns wrong code\n"); > + efi_st_error("Variable was not deleted\n"); > return EFI_ST_FAILURE; > } > /* Enumerate variables */ > -- > 2.34.1 >
Reviewed-by: Ilias Apalodimas <[email protected]> Tested-by: Ilias Apalodimas <[email protected]>
