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

[Masami Hiramatsu]

2021年7月1日(木) 0:30 Heinrich Schuchardt <xypron.glpk at gmx.de>:
>
> On 6/30/21 5:07 PM, Masami Hiramatsu wrote:
> > 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.
>
> That is why I proposed this line. It covers all three test cases.

Ah, thanks for the confirmation. OK, I'll fix with it.

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