[PATCH] efi_loader: Improve the parameter check for QueryVariableInfo()
Improve efi_query_variable_info() to check the parameter settings and return correct error code according to the UEFI spec 2.9.
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); + + 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 */ + 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 */ + ret = EFI_INVALID_PARAMETER; + } else { + ret = efi_query_variable_info_int(attributes, maximum_variable_storage_size, remaining_variable_storage_size, maximum_variable_size); + }
return EFI_EXIT(ret); }
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@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.
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); }
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
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
Hi Heinrich,
2021年6月30日(水) 23:55 Heinrich Schuchardt xypron.glpk@gmx.de:
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."
Ah, I see. so either NON_VOLATILE and RUNTIME_ACCESS must be used with BOOTSERVICE_ACCESS.
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.
OK, so BOOTSERVICE_ACCESS is required. Hmm, but in that case, if we confirm BOOTSERVICE_ACCESS bit is set, should we still need to check NON_VOLATILE and RUNTIME_ACCESS?
I mean
if (!(attr & BOOTSERVICE_ACCESS)) return INVALID_PARAM; else if (...) /* at this point, attr must have the BOOTSERVICE_ACCESS */
Thus, attributes shouldn't be equal to any of;
- EFI_VARIABLE_NON_VOLATILE
- EFI_VARIABLE_RUNTIME_ACCESS
- EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS
So, I think we only need " !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS) " this check.
Best regards,
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
On 6/30/21 5:07 PM, Masami Hiramatsu wrote:
Hi Heinrich,
2021年6月30日(水) 23:55 Heinrich Schuchardt xypron.glpk@gmx.de:
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."
Ah, I see. so either NON_VOLATILE and RUNTIME_ACCESS must be used with BOOTSERVICE_ACCESS.
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.
OK, so BOOTSERVICE_ACCESS is required. Hmm, but in that case, if we confirm BOOTSERVICE_ACCESS bit is set, should we still need to check NON_VOLATILE and RUNTIME_ACCESS?
I mean
if (!(attr & BOOTSERVICE_ACCESS)) return INVALID_PARAM; else if (...) /* at this point, attr must have the BOOTSERVICE_ACCESS */
Thus, attributes shouldn't be equal to any of;
- EFI_VARIABLE_NON_VOLATILE
- EFI_VARIABLE_RUNTIME_ACCESS
- EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS
So, I think we only need " !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)" this check.
That is why I proposed this line. It covers all three test cases.
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
2021年7月1日(木) 0:30 Heinrich Schuchardt xypron.glpk@gmx.de:
On 6/30/21 5:07 PM, Masami Hiramatsu wrote:
Hi Heinrich,
2021年6月30日(水) 23:55 Heinrich Schuchardt xypron.glpk@gmx.de:
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."
Ah, I see. so either NON_VOLATILE and RUNTIME_ACCESS must be used with BOOTSERVICE_ACCESS.
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.
OK, so BOOTSERVICE_ACCESS is required. Hmm, but in that case, if we confirm BOOTSERVICE_ACCESS bit is set, should we still need to check NON_VOLATILE and RUNTIME_ACCESS?
I mean
if (!(attr & BOOTSERVICE_ACCESS)) return INVALID_PARAM; else if (...) /* at this point, attr must have the BOOTSERVICE_ACCESS */
Thus, attributes shouldn't be equal to any of;
- EFI_VARIABLE_NON_VOLATILE
- EFI_VARIABLE_RUNTIME_ACCESS
- EFI_VARIABLE_NON_VOLATILE|EFI_VARIABLE_RUNTIME_ACCESS
So, I think we only need " !(attributes & EFI_VARIABLE_BOOTSERVICE_ACCESS)" this check.
That is why I proposed this line. It covers all three test cases.
Ah, thanks for the confirmation. OK, I'll fix with it.
Best regards,
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
participants (2)
-
Heinrich Schuchardt -
Masami Hiramatsu