diff mbox series

[16/16] efi_selftest: adjust runtime test for variables

Message ID 20200331060739.4570-1-xypron.glpk@gmx.de
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: non-volatile and runtime variables | expand

Commit Message

Heinrich Schuchardt March 31, 2020, 6:07 a.m. UTC 
As variable services are available at runtime we have to expect EFI_SUCCESS
when calling the services.

Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
EFI_VARIABLE_NON_VOLATILE set.

Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
 1 file changed, 39 insertions(+), 8 deletions(-)

--
2.25.1

Comments

AKASHI Takahiro April 1, 2020, 1:05 a.m. UTC | #1 
On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
> As variable services are available at runtime we have to expect EFI_SUCCESS
> when calling the services.

Why not run variables test *after* ExitBootServices as well as
*before* that event?

Then we should have more test cases.

> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
> EFI_VARIABLE_NON_VOLATILE set.

I'm not sure if this claim(check) is true.
At least, you need provide more description about specific test conditions.

-Takahiro Akashi

>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
>  1 file changed, 39 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> index b3b40ad2cf..c6005eeeaf 100644
> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
> 
> -/*
> - * Setup unit test.
> +/**
> + * setup() - set up unit test.
>   *
>   * @handle	handle of the loaded image
>   * @systable	system table
> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
>  /**
>   * execute() - execute unit test
>   *
> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
> + * Test variable services at runtime.
>   */
>  static int execute(void)
>  {
> @@ -52,37 +52,68 @@ static int execute(void)
>  	efi_guid_t guid;
>  	u64 max_storage, rem_storage, max_size;
> 
> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +					   EFI_VARIABLE_RUNTIME_ACCESS |
> +					   EFI_VARIABLE_NON_VOLATILE,
>  					   &max_storage, &rem_storage,
>  					   &max_size);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("QueryVariableInfo failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> 
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				    EFI_VARIABLE_NON_VOLATILE,
> +				    3, v + 4);
> +	if (ret != EFI_INVALID_PARAMETER) {
> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>  				    EFI_VARIABLE_RUNTIME_ACCESS,
>  				    3, v + 4);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_INVALID_PARAMETER) {
> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> +				    EFI_VARIABLE_RUNTIME_ACCESS |
> +				    EFI_VARIABLE_NON_VOLATILE,
> +				    3, v + 4);
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("SetVariable failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> +
>  	len = 3;
>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
>  				    &attr, &len, data);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("GetVariable failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> +
>  	memset(&guid, 0, 16);
>  	*varname = 0;
> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
> -	if (ret != EFI_UNSUPPORTED) {
> +	if (ret != EFI_SUCCESS) {
>  		efi_st_error("GetNextVariableName failed\n");
>  		return EFI_ST_FAILURE;
>  	}
> 
> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
> +				    3, v + 4);
> +	if (ret != EFI_SUCCESS) {
> +		efi_st_error("Variable deletion failed\n");
> +		return EFI_ST_FAILURE;
> +	}
> +
>  	return EFI_ST_SUCCESS;
>  }
> 
> --
> 2.25.1
>
Heinrich Schuchardt April 1, 2020, 6:37 a.m. UTC | #2 
On 4/1/20 3:05 AM, AKASHI Takahiro wrote:
> On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
>> As variable services are available at runtime we have to expect EFI_SUCCESS
>> when calling the services.
>
> Why not run variables test *after* ExitBootServices as well as
> *before* that event?

We have a separate test for before ExitBootServices
(lib/efi_selftest/efi_selftest_variables.c).

>
> Then we should have more test cases.

Please, feel free to supply some.

Unfortunately SCT does not provide runtime testing. I have been running
my code successfully against Ubuntu's firmware test suite
(https://git.launchpad.net/fwts).

>
>> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
>> EFI_VARIABLE_NON_VOLATILE set.
>
> I'm not sure if this claim(check) is true.
> At least, you need provide more description about specific test conditions.

At runtime non-volatile variables and variables without runtime access
cannot be changed according to the UEFI spec.

Best regards

Heinrich

>
> -Takahiro Akashi
>
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
>>  1 file changed, 39 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
>> index b3b40ad2cf..c6005eeeaf 100644
>> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
>> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
>> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
>>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
>>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
>>
>> -/*
>> - * Setup unit test.
>> +/**
>> + * setup() - set up unit test.
>>   *
>>   * @handle	handle of the loaded image
>>   * @systable	system table
>> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
>>  /**
>>   * execute() - execute unit test
>>   *
>> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
>> + * Test variable services at runtime.
>>   */
>>  static int execute(void)
>>  {
>> @@ -52,37 +52,68 @@ static int execute(void)
>>  	efi_guid_t guid;
>>  	u64 max_storage, rem_storage, max_size;
>>
>> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +					   EFI_VARIABLE_RUNTIME_ACCESS |
>> +					   EFI_VARIABLE_NON_VOLATILE,
>>  					   &max_storage, &rem_storage,
>>  					   &max_size);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("QueryVariableInfo failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>>
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +				    EFI_VARIABLE_NON_VOLATILE,
>> +				    3, v + 4);
>> +	if (ret != EFI_INVALID_PARAMETER) {
>> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>  				    EFI_VARIABLE_RUNTIME_ACCESS,
>>  				    3, v + 4);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_INVALID_PARAMETER) {
>> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
>> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>> +				    EFI_VARIABLE_RUNTIME_ACCESS |
>> +				    EFI_VARIABLE_NON_VOLATILE,
>> +				    3, v + 4);
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("SetVariable failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>> +
>>  	len = 3;
>>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
>>  				    &attr, &len, data);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("GetVariable failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>> +
>>  	memset(&guid, 0, 16);
>>  	*varname = 0;
>> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
>>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
>> -	if (ret != EFI_UNSUPPORTED) {
>> +	if (ret != EFI_SUCCESS) {
>>  		efi_st_error("GetNextVariableName failed\n");
>>  		return EFI_ST_FAILURE;
>>  	}
>>
>> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
>> +				    3, v + 4);
>> +	if (ret != EFI_SUCCESS) {
>> +		efi_st_error("Variable deletion failed\n");
>> +		return EFI_ST_FAILURE;
>> +	}
>> +
>>  	return EFI_ST_SUCCESS;
>>  }
>>
>> --
>> 2.25.1
>>
AKASHI Takahiro April 1, 2020, 7:27 a.m. UTC | #3 
On Wed, Apr 01, 2020 at 08:37:38AM +0200, Heinrich Schuchardt wrote:
> On 4/1/20 3:05 AM, AKASHI Takahiro wrote:
> > On Tue, Mar 31, 2020 at 08:07:39AM +0200, Heinrich Schuchardt wrote:
> >> As variable services are available at runtime we have to expect EFI_SUCCESS
> >> when calling the services.
> >
> > Why not run variables test *after* ExitBootServices as well as
> > *before* that event?
> 
> We have a separate test for before ExitBootServices
> (lib/efi_selftest/efi_selftest_variables.c).

Ah, I misundersttood the file to be patched.
(EXECUTE_BEFORE_BOOTTIME_EXIT and SETUP_BEFORE_BOOTTIME_EXIT
are quite  confusing.)

> >
> > Then we should have more test cases.
> 
> Please, feel free to supply some.

?
You re-invented UEFI variable implementation almost from the scratch,
then you should cover as many corner cases as possible in general.

-Takahiro Akashi

> Unfortunately SCT does not provide runtime testing. I have been running
> my code successfully against Ubuntu's firmware test suite
> (https://git.launchpad.net/fwts).
> 
> >
> >> Check the SetVariable() only succeeds with EFI_VARIABLE_RUNTIME_ACCESS and
> >> EFI_VARIABLE_NON_VOLATILE set.
> >
> > I'm not sure if this claim(check) is true.
> > At least, you need provide more description about specific test conditions.
> 
> At runtime non-volatile variables and variables without runtime access
> cannot be changed according to the UEFI spec.

> Best regards
> 
> Heinrich
> 
> >
> > -Takahiro Akashi
> >
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >>  .../efi_selftest_variables_runtime.c          | 47 +++++++++++++++----
> >>  1 file changed, 39 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> index b3b40ad2cf..c6005eeeaf 100644
> >> --- a/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> +++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
> >> @@ -20,8 +20,8 @@ static const efi_guid_t guid_vendor0 =
> >>  	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
> >>  		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);
> >>
> >> -/*
> >> - * Setup unit test.
> >> +/**
> >> + * setup() - set up unit test.
> >>   *
> >>   * @handle	handle of the loaded image
> >>   * @systable	system table
> >> @@ -38,7 +38,7 @@ static int setup(const efi_handle_t img_handle,
> >>  /**
> >>   * execute() - execute unit test
> >>   *
> >> - * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
> >> + * Test variable services at runtime.
> >>   */
> >>  static int execute(void)
> >>  {
> >> @@ -52,37 +52,68 @@ static int execute(void)
> >>  	efi_guid_t guid;
> >>  	u64 max_storage, rem_storage, max_size;
> >>
> >> -	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
> >> +	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +					   EFI_VARIABLE_RUNTIME_ACCESS |
> >> +					   EFI_VARIABLE_NON_VOLATILE,
> >>  					   &max_storage, &rem_storage,
> >>  					   &max_size);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("QueryVariableInfo failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >>
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				    EFI_VARIABLE_NON_VOLATILE,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_INVALID_PARAMETER) {
> >> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >>  	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >>  				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >>  				    EFI_VARIABLE_RUNTIME_ACCESS,
> >>  				    3, v + 4);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_INVALID_PARAMETER) {
> >> +		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
> >> +				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
> >> +				    EFI_VARIABLE_RUNTIME_ACCESS |
> >> +				    EFI_VARIABLE_NON_VOLATILE,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("SetVariable failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >> +
> >>  	len = 3;
> >>  	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
> >>  				    &attr, &len, data);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("GetVariable failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >> +
> >>  	memset(&guid, 0, 16);
> >>  	*varname = 0;
> >> +	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
> >>  	ret = runtime->get_next_variable_name(&len, varname, &guid);
> >> -	if (ret != EFI_UNSUPPORTED) {
> >> +	if (ret != EFI_SUCCESS) {
> >>  		efi_st_error("GetNextVariableName failed\n");
> >>  		return EFI_ST_FAILURE;
> >>  	}
> >>
> >> +	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
> >> +				    3, v + 4);
> >> +	if (ret != EFI_SUCCESS) {
> >> +		efi_st_error("Variable deletion failed\n");
> >> +		return EFI_ST_FAILURE;
> >> +	}
> >> +
> >>  	return EFI_ST_SUCCESS;
> >>  }
> >>
> >> --
> >> 2.25.1
> >>
>
diff mbox series

Patch

diff --git a/lib/efi_selftest/efi_selftest_variables_runtime.c b/lib/efi_selftest/efi_selftest_variables_runtime.c
index b3b40ad2cf..c6005eeeaf 100644
--- a/lib/efi_selftest/efi_selftest_variables_runtime.c
+++ b/lib/efi_selftest/efi_selftest_variables_runtime.c
@@ -20,8 +20,8 @@  static const efi_guid_t guid_vendor0 =
 	EFI_GUID(0x67029eb5, 0x0af2, 0xf6b1,
 		 0xda, 0x53, 0xfc, 0xb5, 0x66, 0xdd, 0x1c, 0xe6);

-/*
- * Setup unit test.
+/**
+ * setup() - set up unit test.
  *
  * @handle	handle of the loaded image
  * @systable	system table
@@ -38,7 +38,7 @@  static int setup(const efi_handle_t img_handle,
 /**
  * execute() - execute unit test
  *
- * As runtime support is not implmented expect EFI_UNSUPPORTED to be returned.
+ * Test variable services at runtime.
  */
 static int execute(void)
 {
@@ -52,37 +52,68 @@  static int execute(void)
 	efi_guid_t guid;
 	u64 max_storage, rem_storage, max_size;

-	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS,
+	ret = runtime->query_variable_info(EFI_VARIABLE_BOOTSERVICE_ACCESS |
+					   EFI_VARIABLE_RUNTIME_ACCESS |
+					   EFI_VARIABLE_NON_VOLATILE,
 					   &max_storage, &rem_storage,
 					   &max_size);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("QueryVariableInfo failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_RUNTIME_ACCESS\n");
+		return EFI_ST_FAILURE;
+	}
+
 	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
 				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
 				    EFI_VARIABLE_RUNTIME_ACCESS,
 				    3, v + 4);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_INVALID_PARAMETER) {
+		efi_st_error("SetVariable succeeded w/o EFI_VARIABLE_NON_VOLATILE\n");
+		return EFI_ST_FAILURE;
+	}
+
+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0,
+				    EFI_VARIABLE_BOOTSERVICE_ACCESS |
+				    EFI_VARIABLE_RUNTIME_ACCESS |
+				    EFI_VARIABLE_NON_VOLATILE,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("SetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	len = 3;
 	ret = runtime->get_variable(L"efi_st_var0", &guid_vendor0,
 				    &attr, &len, data);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetVariable failed\n");
 		return EFI_ST_FAILURE;
 	}
+
 	memset(&guid, 0, 16);
 	*varname = 0;
+	len = EFI_ST_MAX_VARNAME_SIZE * sizeof(u16);
 	ret = runtime->get_next_variable_name(&len, varname, &guid);
-	if (ret != EFI_UNSUPPORTED) {
+	if (ret != EFI_SUCCESS) {
 		efi_st_error("GetNextVariableName failed\n");
 		return EFI_ST_FAILURE;
 	}

+	ret = runtime->set_variable(L"efi_st_var0", &guid_vendor0, 0,
+				    3, v + 4);
+	if (ret != EFI_SUCCESS) {
+		efi_st_error("Variable deletion failed\n");
+		return EFI_ST_FAILURE;
+	}
+
 	return EFI_ST_SUCCESS;
 }