[PATCH] efi_loader: Improve the parameter check for QueryVariableInfo()

[Masami Hiramatsu]

Hi Heinrich,

Thanks for the review!

2021年6月30日(水) 16:06 Heinrich Schuchardt <xypron.glpk at gmx.de>:

>
> On 6/30/21 7:51 AM, Masami Hiramatsu wrote:
> > Improve efi_query_variable_info() to check the parameter settings
> > and return correct error code according to the UEFI spec 2.9.
>
> Detailed requirements can be found in the Self Certification Test (SCT)
> II Case Specification, June 2017, chapter 4.1.4 QueryVariableInfo().

Yes, actually this was found by the SCT.
>
> I would prefer to add that reference.

OK, I'll add it.

>
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> > Reported-by: Kazuhiko Sakamoto <sakamoto.kazuhiko at socionext.com>
> > ---
> >   lib/efi_loader/efi_var_common.c |   20 +++++++++++++++++++-
> >   1 file changed, 19 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> > index 83479dd142..62aa7f970c 100644
> > --- a/lib/efi_loader/efi_var_common.c
> > +++ b/lib/efi_loader/efi_var_common.c
> > @@ -163,10 +163,28 @@ efi_status_t EFIAPI efi_query_variable_info(
> >       EFI_ENTRY("%x %p %p %p", attributes, maximum_variable_storage_size,
> >                 remaining_variable_storage_size, maximum_variable_size);
> >
> > -     ret = efi_query_variable_info_int(attributes,
> > +     if (attributes == 0 || maximum_variable_storage_size == NULL ||
> > +         remaining_variable_storage_size == NULL ||
> > +         maximum_variable_size == NULL)
> > +             return EFI_EXIT(EFI_INVALID_PARAMETER);
>
> scripts/checkpatch.pl:
>
> CHECK: Comparison to NULL could be written "!maximum_variable_storage_size"
> #26: FILE: lib/efi_loader/efi_var_common.c:166:
> +       if (attributes == 0 || maximum_variable_storage_size == NULL ||
>
> Also use !attributes instead of attributes == 0.

OK.

>
> CHECK: Comparison to NULL could be written
> "!remaining_variable_storage_size"
> #27: FILE: lib/efi_loader/efi_var_common.c:167:
> +           remaining_variable_storage_size == NULL ||
>
> CHECK: Comparison to NULL could be written "!maximum_variable_size"
> #28: FILE: lib/efi_loader/efi_var_common.c:168:
> +           maximum_variable_size == NULL)
>
> > +
> > +     if ((attributes & ~(u32)EFI_VARIABLE_MASK) ||
> > +         (attributes & EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS) ||
> > +         (!IS_ENABLED(CONFIG_EFI_SECURE_BOOT) &&
> > +          (attributes & EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS))) {
> > +             ret = EFI_UNSUPPORTED;
> > +     } else if ((attributes & (EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS)) == EFI_VARIABLE_RUNTIME_ACCESS) {
> > +             /* Runtime accessible variable must also be accessible in bootservices */
>
> Please, stick to 80 characters per line.

OK.

>
> This does not catch attributes == EFI_VARIABLE_NON_VOLATILE where test
> case 5.2.1.4.5 requires EFI_INVALID_PARAMETER.

Hmm, but this could pass the SCT runtime test.
So attributes == EFI_VARIABLES_NON_VOLATILE should fail?
Actually, I referred to the VariableServiceQueryVariableInfo()@EDK2
and UEFI spec,
and I couldn't see such conditions.

>
> Shouldn't we check
>
>         !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)
>
> instead?

Ah, right, because this function is only used for the bootservice.
(I found that runtime service uses another function)

>
> > +             ret = EFI_INVALID_PARAMETER;
> > +     } else if ((attributes & (EFI_VARIABLE_NON_VOLATILE | EFI_VARIABLE_HARDWARE_ERROR_RECORD)) == EFI_VARIABLE_HARDWARE_ERROR_RECORD) {
> > +             /* HW error occurs only on non-volatile variables */
>
> We don't support EFI_VARIABLE_HARDWARE_ERROR_RECORD at all. So shouldn't
> flag EFI_VARIABLE_HARDWARE_ERROR_RECORD result in EFI_UNSUPPORTED?

OK, I'll do.

BTW, from the UEFI spec, EFI_VARIABLE_APPEND_WRITE should be ignored
in the QueryVariableInfo because that flag is used for SetVariables
(as overwrite flag).
Thus, if attributes == EFI_VARIABLE_APPEND_WRITE, should we return
EFI_INVALID_PARAMETER?

>
> > +             ret = EFI_INVALID_PARAMETER;
> > +     } else {
> > +             ret = efi_query_variable_info_int(attributes,
> >                                         maximum_variable_storage_size,
> >                                         remaining_variable_storage_size,
> >                                         maximum_variable_size);
>
> CHECK: Alignment should match open parenthesis
> #44: FILE: lib/efi_loader/efi_var_common.c:184:
> +               ret = efi_query_variable_info_int(attributes,
>                                            maximum_variable_storage_size,

OK, let me fix that.

Thank you,

>
> Best regards
>
> Heinrich
>
> > +     }
> >
> >       return EFI_EXIT(ret);
> >   }
> >
>


--
Masami Hiramatsu