On 6/30/21 11:32 AM, Masami Hiramatsu wrote:
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.
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."
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.
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