Hi Heinrich, Thanks for the review!
2021年6月30日(水) 16:06 Heinrich Schuchardt <[email protected]>: > > 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 <[email protected]> > > Reported-by: Kazuhiko Sakamoto <[email protected]> > > --- > > 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
