Re: [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.g...@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