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

[Masami Hiramatsu]

Hi Heinrich,

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

> >> 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.
>
> The SCT specification requires in 5.2.1.4.5.:
>
> "1. Call QueryVariableInfoservice with the Attributes:
>
> * EFI_VARIABLE_NON_VOLATILE
> * EFI_VARIABLE_RUNTIME_ACCESS
> * EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS
>
> The returned code must be EFI_INVALID_PARAMETER."

Ah, I see. so either NON_VOLATILE and RUNTIME_ACCESS must be used with
BOOTSERVICE_ACCESS.

>
> See patch
>
> [PATCH edk2-test 1/1] uefi-sct/SctPkg: uefi-sct:
> QueryVariableInfo(EFI_VARIABLE_NON_VOLATILE)
> https://edk2.groups.io/g/devel/message/77367
>
> >
> >>
> >> 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)
>
> A variable without EFI_VARIABLE_BOOTSERVICE_ACCESS could neither be
> accessed before nor after ExitBootServices(). So this has to be invalid.

OK, so BOOTSERVICE_ACCESS is required.
Hmm, but in that case, if we confirm BOOTSERVICE_ACCESS bit is set,
should we still need to check NON_VOLATILE and RUNTIME_ACCESS?

I mean

if (!(attr & BOOTSERVICE_ACCESS))
    return INVALID_PARAM;
else if (...) /* at this point, attr must have the BOOTSERVICE_ACCESS */

Thus, attributes shouldn't be equal to any of;

> * EFI_VARIABLE_NON_VOLATILE
> * EFI_VARIABLE_RUNTIME_ACCESS
> * EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS

So, I think we only need
" !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) "
this check.

Best regards,

>
> Best regards
>
> Heinrich
>
> >
> >>
> >>> +             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
> >
>


-- 
Masami Hiramatsu