[PATCH] uefirtvariable: remove the invalid attribute tests after ExitBootServices() is performed
Colin Ian King
colin.king at canonical.com
Wed Dec 19 11:00:43 UTC 2012
On 18/12/12 06:15, IvanHu wrote:
> 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?
Sounds sensible to me. Let's see if any firmware fails this new test ;-)
>
> 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