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

[Heinrich Schuchardt]

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().

I would prefer to add that reference.

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

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.

This does not catch attributes == EFI_VARIABLE_NON_VOLATILE where test
case 5.2.1.4.5 requires EFI_INVALID_PARAMETER.

Shouldn't we check

	!(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)

instead?

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

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

Best regards

Heinrich

> +	}
>
>   	return EFI_EXIT(ret);
>   }
>