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

[Heinrich Schuchardt]

On 6/30/21 11:32 AM, Masami Hiramatsu wrote:
> 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.

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

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.

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
>