efi_loader: Improve the parameter check for QueryVariableInfo()

Message ID 162503230398.71097.12252874424030046803.stgit@localhost
State New
Headers show
Series
  • efi_loader: Improve the parameter check for QueryVariableInfo()
Related show

Commit Message

Masami Hiramatsu June 30, 2021, 5:51 a.m.
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(-)

Comments

Heinrich Schuchardt June 30, 2021, 7:06 a.m. | #1
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);

>   }

>
Masami Hiramatsu June 30, 2021, 9:32 a.m. | #2
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
Heinrich Schuchardt June 30, 2021, 2:55 p.m. | #3
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

>
Masami Hiramatsu June 30, 2021, 3:07 p.m. | #4
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

> >

>



-- 
Masami Hiramatsu
Heinrich Schuchardt June 30, 2021, 3:30 p.m. | #5
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

>>>

>>

>

>
Masami Hiramatsu June 30, 2021, 3:32 p.m. | #6
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

> >>>

> >>

> >

> >

>



-- 
Masami Hiramatsu

Patch

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);
 }