Hi Heinrich,
Thanks for the review!
2021年6月30日(水) 16:06 Heinrich Schuchardt xypron.glpk@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@linaro.org Reported-by: Kazuhiko Sakamoto sakamoto.kazuhiko@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.
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)
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