[PATCH] uefirtvariable: remove the invalid attribute tests after ExitBootServices() is performed
IvanHu
ivan.hu at canonical.com
Tue Dec 18 06:15:15 UTC 2012
On 12/14/2012 06:52 PM, Colin Ian King wrote:
> On 14/12/12 09:48, Ivan Hu wrote:
>> In the UEFI spec Version 2.3.1, Errata C, section 7.2, it mentions
>> that attributes have some usage rules:
>> * Attributes that have EFI_VARIABLE_RUNTIME_ACCESS set must also have
>> EFI_VARIABLE_BOOTSERVICE_ACCESS set.
>>
>> * Once ExitBootServices() is performed, only variables that have
>> EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set can be
>> set with SetVariable().
>
> I'm still a bit uncertain about this.
>
> So what happens if we try to set variables with the wrong attributes?
> Should the firmware return an error? In which case, isn't it valid to
> test with invalid attributes to make sure the firmware rejects this?
>
> Colin
>
Hi Colin,
the UEFI spec Version 2.3.1, Errata C p.223, related to the setvariable
are below:
• Runtime access to a data variable implies boot service access.
Attributes that have
EFI_VARIABLE_RUNTIME_ACCESS set must also have
EFI_VARIABLE_BOOTSERVICE_ACCESS set. The caller is responsible for
following this rule.
• Once ExitBootServices() is performed, data variables that did not have
EFI_VARIABLE_RUNTIME_ACCESS set are no longer visible to GetVariable().
• Once ExitBootServices() is performed, only variables that have
EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE set can be
set with SetVariable(). Variables that have runtime access but that
are not nonvolatile are read-only data variables once ExitBootServices()
is performed.
When ExitBootServices is performed(power on to Ubuntu), the POST
resource of firmware will be released, only runtime resource reserved.
But, actually UEFI spec doesn't tell what should be done for firmware
when the invalid attribute is set under this situation. So the return
status depends on firmware implementation. Checking with the returen
status on spec, *EFI_INVALID_PARAMETER - An invalid combination of
attribute bits was supplied, or the DataSize exceeds the maximum
allowed.* should be the most likely be returned by most firmware. I
built the EDK2, and powered on and tested the Bios with Qemu. It did
return EFI_INVALID_PARAMETER. So I think it's worth to add another patch
to test the invalid attribute in SetVariable function test, subtest3, to
check and give warnings or failure if firmware returns EFI_SUCCESS under
this situation. Make sense?
Cheers,
Ivan
>>
>> Once power on to Ubuntu, the attribute which sets
>> EFI_VARIABLE_BOOTSERVICE_ACCESS,
>> EFI_VARIABLE_RUNTIME_ACCESS and EFI_VARIABLE_NON_VOLATILE, should only
>> be tests.
>>
>> Signed-off-by: Ivan Hu <ivan.hu at canonical.com>
>> ---
>> src/uefi/uefirtvariable/uefirtvariable.c | 92
>> ++++++++++++------------------
>> 1 file changed, 35 insertions(+), 57 deletions(-)
>>
>> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c
>> b/src/uefi/uefirtvariable/uefirtvariable.c
>> index d87253e..d9e91d2 100644
>> --- a/src/uefi/uefirtvariable/uefirtvariable.c
>> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
>> @@ -48,11 +48,8 @@
>> static int fd;
>> EFI_GUID gtestguid1 = TEST_GUID1;
>> EFI_GUID gtestguid2 = TEST_GUID2;
>> -uint32_t attributesarray[] = { FWTS_UEFI_VAR_BOOTSERVICE_ACCESS,
>> - FWTS_UEFI_VAR_NON_VOLATILE |
>> FWTS_UEFI_VAR_BOOTSERVICE_ACCESS,
>> - FWTS_UEFI_VAR_BOOTSERVICE_ACCESS |
>> FWTS_UEFI_VAR_RUNTIME_ACCESS,
>> - FWTS_UEFI_VAR_NON_VOLATILE |
>> FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | FWTS_UEFI_VAR_RUNTIME_ACCESS};
>>
>> +uint32_t attributes = FWTS_UEFI_VAR_NON_VOLATILE |
>> FWTS_UEFI_VAR_BOOTSERVICE_ACCESS | FWTS_UEFI_VAR_RUNTIME_ACCESS;
>> uint16_t variablenametest[] = {'T', 'e', 's', 't', 'v', 'a', 'r',
>> '\0'};
>>
>> static int uefirtvariable_init(fwts_framework *fw)
>> @@ -86,14 +83,14 @@ static int uefirtvariable_deinit(fwts_framework *fw)
>> return FWTS_OK;
>> }
>>
>> -static int getvariable_test(fwts_framework *fw, uint32_t attributes,
>> uint64_t datasize, uint16_t *varname)
>> +static int getvariable_test(fwts_framework *fw, uint64_t datasize,
>> uint16_t *varname)
>> {
>> long ioret;
>> struct efi_getvariable getvariable;
>> struct efi_setvariable setvariable;
>>
>> uint64_t status;
>> - uint8_t testdata[datasize+1];
>> + uint8_t testdata[MAX_DATA_LENGTH];
>> uint64_t dataindex;
>> uint64_t getdatasize;
>> uint32_t attributestest;
>> @@ -101,7 +98,7 @@ static int getvariable_test(fwts_framework *fw,
>> uint32_t attributes, uint64_t da
>> uint8_t data[datasize+1];
>> for (dataindex = 0; dataindex < datasize; dataindex++)
>> data[dataindex] = (uint8_t)dataindex;
>> - data[dataindex] = '0';
>> + data[dataindex] = '\0';
>>
>> setvariable.VariableName = varname;
>> setvariable.VendorGuid = >estguid1;
>> @@ -172,7 +169,6 @@ static int getvariable_test(fwts_framework *fw,
>> uint32_t attributes, uint64_t da
>> return FWTS_OK;
>> }
>>
>> -
>> static bool compare_guid(EFI_GUID *guid1, EFI_GUID *guid2)
>> {
>> bool ident = true;
>> @@ -205,7 +201,7 @@ static bool compare_name(uint16_t *name1, uint16_t
>> *name2)
>> return ident;
>> }
>>
>> -static int getnextvariable_test(fwts_framework *fw, uint32_t attributes)
>> +static int getnextvariable_test(fwts_framework *fw)
>> {
>> long ioret;
>> uint64_t status;
>> @@ -223,6 +219,7 @@ static int getnextvariable_test(fwts_framework
>> *fw, uint32_t attributes)
>>
>> for (dataindex = 0; dataindex < datasize; dataindex++)
>> data[dataindex] = (uint8_t)dataindex;
>> + data[dataindex] = '\0';
>>
>> setvariable.VariableName = variablenametest;
>> setvariable.VendorGuid = >estguid1;
>> @@ -306,7 +303,7 @@ static int
>> setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, u
>>
>> for (dataindex = 0; dataindex < datasize; dataindex++)
>> data[dataindex] = (uint8_t)dataindex + datadiff;
>> - data[dataindex] = '0';
>> + data[dataindex] = '\0';
>>
>> setvariable.VariableName = varname;
>> setvariable.VendorGuid = gtestguid;
>> @@ -325,7 +322,7 @@ static int
>> setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, u
>> return FWTS_OK;
>> }
>>
>> -static int setvariable_checkvariable(fwts_framework *fw, uint32_t
>> attributes, uint64_t datasize,
>> +static int setvariable_checkvariable(fwts_framework *fw, uint64_t
>> datasize,
>> uint16_t *varname, EFI_GUID *gtestguid, uint8_t
>> datadiff)
>> {
>> long ioret;
>> @@ -404,7 +401,7 @@ static int
>> setvariable_checkvariable_notfound(fwts_framework *fw, uint16_t *varn
>> return FWTS_ERROR;
>> }
>>
>> -static int setvariable_test1(fwts_framework *fw, uint32_t attributes,
>> uint64_t datasize1,
>> +static int setvariable_test1(fwts_framework *fw, uint64_t datasize1,
>> uint64_t datasize2, uint16_t *varname)
>> {
>> uint8_t datadiff_g2 = 2, datadiff_g1 = 0;
>> @@ -417,11 +414,11 @@ static int setvariable_test1(fwts_framework *fw,
>> uint32_t attributes, uint64_t d
>> >estguid1, datadiff_g1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize2, varname,
>> + if (setvariable_checkvariable(fw, datasize2, varname,
>> >estguid2, datadiff_g2) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize1, varname,
>> + if (setvariable_checkvariable(fw, datasize1, varname,
>> >estguid1, datadiff_g1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> @@ -436,7 +433,7 @@ static int setvariable_test1(fwts_framework *fw,
>> uint32_t attributes, uint64_t d
>> return FWTS_OK;
>> }
>>
>> -static int setvariable_test2(fwts_framework *fw, uint32_t attributes,
>> uint16_t *varname)
>> +static int setvariable_test2(fwts_framework *fw, uint16_t *varname)
>> {
>> uint64_t datasize = 10;
>> uint8_t datadiff1 = 0, datadiff2 = 2, datadiff3 = 4;
>> @@ -450,7 +447,7 @@ static int setvariable_test2(fwts_framework *fw,
>> uint32_t attributes, uint16_t *
>> >estguid1, datadiff1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize, varname,
>> + if (setvariable_checkvariable(fw, datasize, varname,
>> >estguid1, datadiff1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> @@ -466,7 +463,7 @@ static int setvariable_test2(fwts_framework *fw,
>> uint32_t attributes, uint16_t *
>> >estguid1, datadiff2) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize, varname,
>> + if (setvariable_checkvariable(fw, datasize, varname,
>> >estguid1, datadiff2) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> @@ -480,7 +477,7 @@ static int setvariable_test2(fwts_framework *fw,
>> uint32_t attributes, uint16_t *
>> >estguid1, datadiff3) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize, varname,
>> + if (setvariable_checkvariable(fw, datasize, varname,
>> >estguid1, datadiff3) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> @@ -491,7 +488,7 @@ static int setvariable_test2(fwts_framework *fw,
>> uint32_t attributes, uint16_t *
>> return FWTS_OK;
>> }
>>
>> -static int setvariable_test3(fwts_framework *fw, uint32_t attributes)
>> +static int setvariable_test3(fwts_framework *fw)
>> {
>> uint64_t datasize = 10;
>> uint8_t datadiff1 = 0, datadiff2 = 1, datadiff3 = 2;
>> @@ -510,15 +507,15 @@ static int setvariable_test3(fwts_framework *fw,
>> uint32_t attributes)
>> >estguid1, datadiff1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize,
>> variablenametest2,
>> + if (setvariable_checkvariable(fw, datasize, variablenametest2,
>> >estguid1, datadiff2) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize,
>> variablenametest3,
>> + if (setvariable_checkvariable(fw, datasize, variablenametest3,
>> >estguid1, datadiff3) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> - if (setvariable_checkvariable(fw, attributes, datasize,
>> variablenametest,
>> + if (setvariable_checkvariable(fw, datasize, variablenametest,
>> >estguid1, datadiff1) == FWTS_ERROR)
>> return FWTS_ERROR;
>>
>> @@ -537,7 +534,7 @@ static int setvariable_test3(fwts_framework *fw,
>> uint32_t attributes)
>> return FWTS_OK;
>> }
>>
>> -static int setvariable_test4(fwts_framework *fw, uint32_t attributes)
>> +static int setvariable_test4(fwts_framework *fw)
>> {
>> uint64_t datasize = 10;
>> uint8_t datadiff = 0;
>> @@ -556,7 +553,7 @@ static int setvariable_test4(fwts_framework *fw,
>> uint32_t attributes)
>> return FWTS_OK;
>> }
>>
>> -static int setvariable_test5(fwts_framework *fw, uint32_t attributes)
>> +static int setvariable_test5(fwts_framework *fw)
>> {
>> uint64_t datasize = 10;
>> uint8_t datadiff = 0;
>> @@ -577,13 +574,10 @@ static int setvariable_test5(fwts_framework *fw,
>> uint32_t attributes)
>>
>> static int uefirtvariable_test1(fwts_framework *fw)
>> {
>> - uint64_t index;
>> uint64_t datasize = 10;
>>
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (getvariable_test(fw, attributesarray[index], datasize,
>> variablenametest) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (getvariable_test(fw, datasize, variablenametest) == FWTS_ERROR)
>> + return FWTS_ERROR;
>>
>> fwts_passed(fw, "UEFI runtime service GetVariable interface test
>> passed.");
>>
>> @@ -592,12 +586,8 @@ static int uefirtvariable_test1(fwts_framework *fw)
>>
>> static int uefirtvariable_test2(fwts_framework *fw)
>> {
>> - uint64_t index;
>> -
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (getnextvariable_test(fw, attributesarray[index]) ==
>> FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (getnextvariable_test(fw) == FWTS_ERROR)
>> + return FWTS_ERROR;
>>
>> fwts_passed(fw, "UEFI runtime service GetNextVariableName
>> interface test passed.");
>>
>> @@ -606,43 +596,31 @@ static int uefirtvariable_test2(fwts_framework *fw)
>>
>> static int uefirtvariable_test3(fwts_framework *fw)
>> {
>> - uint64_t index;
>> uint64_t datasize1 = 10, datasize2 = 20;
>>
>> fwts_log_info(fw, "Testing SetVariable on two different GUIDs
>> and the same variable name.");
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (setvariable_test1(fw, attributesarray[index], datasize1,
>> datasize2,
>> - variablenametest) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (setvariable_test1(fw, datasize1, datasize2, variablenametest)
>> == FWTS_ERROR)
>> + return FWTS_ERROR;
>> fwts_passed(fw, "SetVariable on two different GUIDs and the same
>> variable name passed.");
>>
>> fwts_log_info(fw, "Testing SetVariable on the same and different
>> variable data.");
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (setvariable_test2(fw, attributesarray[index],
>> variablenametest) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (setvariable_test2(fw, variablenametest) == FWTS_ERROR)
>> + return FWTS_ERROR;
>> fwts_passed(fw, "SetVariable on the same and different variable
>> data passed.");
>>
>> fwts_log_info(fw, "Testing SetVariable on similar variable name.");
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (setvariable_test3(fw, attributesarray[index]) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (setvariable_test3(fw) == FWTS_ERROR)
>> + return FWTS_ERROR;
>> fwts_passed(fw, "SetVariable on similar variable name passed.");
>>
>> fwts_log_info(fw, "Testing SetVariable on DataSize is 0.");
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (setvariable_test4(fw, attributesarray[index]) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (setvariable_test4(fw) == FWTS_ERROR)
>> + return FWTS_ERROR;
>> fwts_passed(fw, "SetVariable on DataSize is 0 passed.");
>>
>> fwts_log_info(fw, "Testing SetVariable on Attributes is 0.");
>> - for (index = 0; index < (sizeof(attributesarray)/(sizeof
>> attributesarray[0])); index++) {
>> - if (setvariable_test5(fw, attributesarray[index]) == FWTS_ERROR)
>> - return FWTS_ERROR;
>> - }
>> + if (setvariable_test5(fw) == FWTS_ERROR)
>> + return FWTS_ERROR;
>> fwts_passed(fw, "SetVariable on Attributes is 0 passed.");
>>
>> return FWTS_OK;
>>
>
>
More information about the fwts-devel
mailing list