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

[Heinrich Schuchardt]

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.

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
>>>
>>
>
>