[PATCH v2 4/4] efi_selftest: add tests for setvariableRT

[Ilias Apalodimas]

Hi Heinrich,


[...]

> >
> > +     memset(v2, 0x1, sizeof(v2));
> >       ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> >                                          &max_storage, &rem_storage,
> >                                          &max_size);
> > @@ -63,10 +69,107 @@ static int execute(void)
> >                                   EFI_VARIABLE_RUNTIME_ACCESS,
> >                                   3, v + 4);
> >       if (IS_ENABLED(CONFIG_EFI_RT_VOLATILE_STORE)) {
> > +             efi_uintn_t prev_len, delta;
> > +
> >               if (ret != EFI_INVALID_PARAMETER) {
> >                       efi_st_error("SetVariable failed\n");
> >                       return EFI_ST_FAILURE;
> >               }
> > +
> > +             len = sizeof(data);
> > +             ret = runtime->get_variable(u"RTStorageVolatile",
> > +                                         &efi_rt_var_guid,
> > +                                         &attr, &len, data);
> > +             if (ret != EFI_SUCCESS) {
> > +                     efi_st_error("GetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +
> > +             len = sizeof(data2);
> > +             ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > +                                         &attr, &len, data2);
> > +             if (ret != EFI_SUCCESS) {
> > +                     efi_st_error("GetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +             /*
> > +              * VarToFile will size must change once a variable is inserted
> > +              * Store it now, we'll use it later
> > +              */
> > +             prev_len = len;
> > +             ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > +                                         EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                         EFI_VARIABLE_RUNTIME_ACCESS |
> > +                                         EFI_VARIABLE_NON_VOLATILE,
> > +                                         sizeof(v2),
> > +                                         v2);
> > +             /*
> > +              * This will try to update VarToFile as well and must fail,
> > +              * without changing or deleting VarToFile
> > +              */
> > +             if (ret != EFI_OUT_OF_RESOURCES) {
> > +                     efi_st_error("SetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +             len = sizeof(data2);
> > +             ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > +                                         &attr, &len, data2);
> > +             if (ret != EFI_SUCCESS || prev_len != len) {
> > +                     efi_st_error("Get/SetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +
> > +             /* SetVariableRT updates VarToFile with efi_st_var0 */
> > +             ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > +                                         EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                         EFI_VARIABLE_RUNTIME_ACCESS |
> > +                                         EFI_VARIABLE_NON_VOLATILE,
> > +                                         sizeof(v), v);
>
> It is fine to test with variable name size (24) and variable value size
> (16) both being multiples of 8. But this does not cover the generic case.

We already test SetVariable with a non-aligned variable at the start.
I'll keep this as is and add append 1byte at the last test as you suggested.

>
> > +             if (ret != EFI_SUCCESS) {
> > +                     efi_st_error("SetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
> > +             len = sizeof(data2);
> > +             delta = 2 * (u16_strlen(u"efi_st_var0") + 1) + sizeof(v) +
> > +                     sizeof(struct efi_var_entry);
>
> In the file all variable are stored at 8 byte aligned positions.
> I am missing ALIGN(,8) here.

Ah yes

>
> > +             ret = runtime->get_variable(u"VarToFile", &efi_rt_var_guid,
> > +                                         &attr, &len, data2);
> > +             if (ret != EFI_SUCCESS || prev_len + delta != len) {
> > +                     efi_st_error("Get/SetVariable failed\n");
> > +                     return EFI_ST_FAILURE;
> > +             }
>
> We should check the header fields of VarToFile (reserved, magic, length,
> crc32), e.g.
>
> struct efi_var_file *hdr = (void *)data2;
> if (memcmp(hdr->magic, EFI_VAR_FILE_MAGIC, 8) ||
>      len != ALIGN(hdr->length + sizeof(hdr), 8) ||
>      ... )

Sure, good idea,

>
> > +
> > +             /* append on an existing variable will updateVarToFile */
> > +             ret = runtime->set_variable(u"efi_st_var0", &guid_vendor0,
> > +                                         EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +                                         EFI_VARIABLE_RUNTIME_ACCESS |
> > +                                         EFI_VARIABLE_APPEND_WRITE |
> > +                                         EFI_VARIABLE_NON_VOLATILE,
> > +                                         sizeof(v), v);
>
> How about just appending 1 byte when using EFI_VARIABLE_APPEND_WRITE and
> checking that the delta here too?

Yes, I'll change this

[...]

Thanks
/Ilias