Re: [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.hirama...@linaro.org>
Reported-by: Kazuhiko Sakamoto <sakamoto.kazuh...@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);
  }