[PATCH] efi_loader: check query_variable_info attributes

[Heinrich Schuchardt]

On 4/29/21 7:46 PM, Vincent Stehlé wrote:
> QueryVariableInfo() must return EFI_INVALID_PARAMETER when an invalid
> combination of attribute bits is supplied.
>
> This fixes three SCT QueryVariableInfo_Conf failures.
>
> Signed-off-by: Vincent Stehlé <vincent.stehle at arm.com>
> Reviewed-by: Grant Likely <grant.likely at arm.com>
> Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> Cc: Alexander Graf <agraf at csgraf.de>
>
> Changes since v1:
> - Remove if/else and return directly
> ---
>   lib/efi_loader/efi_var_common.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/lib/efi_loader/efi_var_common.c b/lib/efi_loader/efi_var_common.c
> index b11ed91a74a..cbf8685fad5 100644
> --- a/lib/efi_loader/efi_var_common.c
> +++ b/lib/efi_loader/efi_var_common.c
> @@ -160,6 +160,10 @@ 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);
>
> +	if (!attributes || ((attributes & EFI_VARIABLE_RUNTIME_ACCESS) &&
> +			    !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)))
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);

Thank you for looking into closing this UEFI compliance gap.

The checks must be executed at runtime too. efi_query_variable_info()
only covers boot time.

StandaloneMM has its own check in RuntimeServiceQueryVariableInfo().
That function only checks that attributes is non-zero. If you consider
that check wrong, you would have to fix it there.

Your U-Boot side checks should be put into efi_query_variable_info_int().

Why should we consider the following values valid?

     attributes == EFI_VARIABLE_APPEND_WRITE
     attributes == EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS
     attributes == EFI_VARIABLE_HARDWARE_ERROR_RECORD
     attributes == 0xffffff00

EFI_VARIABLE_APPEND_WRITE shall be ignored according to chapter 8.2
"Variable Services". Shouldn't we remove the bit before checking the
remaining value?

EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS for a variable that is neither
BS nor RT makes little sense.

According to chapter 8.2.4.2, "Hardware Error Record Variables" a
hardware error variable must be NV, BS, RT, HR.

0xffffff00 has bits set that can never appear in attributes.

efi_set_variable_int() would also accept some invalid combinations of
flags. You could try to extract a common function for checking.

Best regards

Heinrich

> +
>   	ret = efi_query_variable_info_int(attributes,
>   					  maximum_variable_storage_size,
>   					  remaining_variable_storage_size,
>