| Message ID | 162503230398.71097.12252874424030046803.stgit@localhost |
|---|---|
| State | Changes Requested |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | efi_loader: Improve the parameter check for QueryVariableInfo() | expand |
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 > >>> > >> > > > > >
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); }
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(-)