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

[Ilias Apalodimas]

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?

>         }
>
> @@ -329,7 +326,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..0c2c76599e 100644
> --- a/lib/efi_selftest/efi_selftest_variables.c
> +++ b/lib/efi_selftest/efi_selftest_variables.c
> @@ -131,13 +131,39 @@ 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 */
>         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;

This used to fail as well [0]. Does it work now?

>         }
>         /* Enumerate variables */
> --
> 2.34.1
>

[0] https://lore.kernel.org/u-boot/CAC_iWjL2G2mvQb6WNGLTxr+xj64xrYuOKr8G3-3z8Lv7yX5XSg@mail.gmail.com/

Regards
/Ilias