| Message ID | 1355478538-25351-1-git-send-email-ivan.hu@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
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 > > 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@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; >
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@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; >> > >
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@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; >>> >> >> > >
On 12/19/2012 07:00 PM, Colin Ian King wrote: > 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 ;-) Thanks!. I've sent another patch to the setvariable with invalid attributes. I think, this patch is also needed for removing the setvariable fail within getvariable, getnextvaribal and setvariable function tests.
On Fri, Dec 14, 2012 at 5:48 PM, Ivan Hu <ivan.hu@canonical.com> 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(). > > 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@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; > -- > 1.7.10.4 > > If there are more comments and brief description of each test, it will be much better. Acked-by: Keng-Yu Lin <kengyu@canonical.com>
On 12/14/2012 05:48 PM, 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(). > > 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@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; > Acked-by: Alex Hung <alex.hung@canonical.com>
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;
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(). 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@canonical.com> --- src/uefi/uefirtvariable/uefirtvariable.c | 92 ++++++++++++------------------ 1 file changed, 35 insertions(+), 57 deletions(-)