| Message ID | 512B31A0.3070907@canonical.com |
|---|---|
| State | Accepted |
| Headers | show |
On 25/02/13 09:40, Colin Ian King wrote: > Fwd, we should pick this patch up. > > Colin > > -------- Original Message -------- > > X-Scanned-By: MailControl 13381.65 (www.mailcontrol.com) on 10.74.0.136 > > From: Matt Fleming <matt.fleming@intel.com> > > When calling GetVariable(), we need to fill out DataSize with the size > of the Data buffer instead of using whatever garbage value is left on > the stack. > > Furthermore, the Data buffers are not strings in any of the tests, so > there's no need to NUL-terminate them or make them 1-byte larger than > datasize, e.g. there's no real need to do this, > > uint8_t data[datasize+1; > > since the last element of data[] is never read back anyway. > > Cc: Ivan Hu <ivan.hu@canonical.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: Alex Hung <alex.hung@canonical.com> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > > Folks, is there a mailing list for the development of fwts? > > src/uefi/uefirtvariable/uefirtvariable.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c > b/src/uefi/uefirtvariable/uefirtvariable.c > index e8aa041..e00b343 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -89,15 +89,14 @@ static int getvariable_test(fwts_framework *fw, > uint64_t datasize, uint16_t *var > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > uint32_t i; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = >estguid1; > @@ -245,7 +244,6 @@ static int getnextvariable_test(fwts_framework *fw) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > @@ -350,11 +348,10 @@ static int > setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, u > uint64_t status; > uint64_t dataindex; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -392,9 +389,9 @@ static int setvariable_checkvariable(fwts_framework > *fw, uint64_t datasize, > struct efi_getvariable getvariable; > > uint64_t status; > - uint8_t testdata[datasize+1]; > + uint8_t testdata[datasize]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -442,7 +439,7 @@ static int > setvariable_checkvariable_notfound(fwts_framework *fw, uint16_t *varn > > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -472,11 +469,10 @@ static int setvariable_invalidattr(fwts_framework > *fw, uint32_t attributes, uint > struct efi_setvariable setvariable; > uint64_t status; > uint64_t dataindex; > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -775,7 +771,6 @@ static int getnextvariable_multitest(fwts_framework > *fw, uint32_t multitesttime) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; Acked-by: Colin Ian King <colin.king@canonical.com>
On 02/25/2013 05:40 PM, Colin Ian King wrote: > Fwd, we should pick this patch up. > > Colin > > -------- Original Message -------- > > X-Scanned-By: MailControl 13381.65 (www.mailcontrol.com) on 10.74.0.136 > > From: Matt Fleming <matt.fleming@intel.com> > > When calling GetVariable(), we need to fill out DataSize with the size > of the Data buffer instead of using whatever garbage value is left on > the stack. > > Furthermore, the Data buffers are not strings in any of the tests, so > there's no need to NUL-terminate them or make them 1-byte larger than > datasize, e.g. there's no real need to do this, > > uint8_t data[datasize+1; > > since the last element of data[] is never read back anyway. > > Cc: Ivan Hu <ivan.hu@canonical.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: Alex Hung <alex.hung@canonical.com> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > > Folks, is there a mailing list for the development of fwts? > > src/uefi/uefirtvariable/uefirtvariable.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c > b/src/uefi/uefirtvariable/uefirtvariable.c > index e8aa041..e00b343 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -89,15 +89,14 @@ static int getvariable_test(fwts_framework *fw, > uint64_t datasize, uint16_t *var > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > uint32_t i; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = >estguid1; > @@ -245,7 +244,6 @@ static int getnextvariable_test(fwts_framework *fw) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > @@ -350,11 +348,10 @@ static int > setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, u > uint64_t status; > uint64_t dataindex; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -392,9 +389,9 @@ static int setvariable_checkvariable(fwts_framework > *fw, uint64_t datasize, > struct efi_getvariable getvariable; > > uint64_t status; > - uint8_t testdata[datasize+1]; > + uint8_t testdata[datasize]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -442,7 +439,7 @@ static int > setvariable_checkvariable_notfound(fwts_framework *fw, uint16_t *varn > > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -472,11 +469,10 @@ static int setvariable_invalidattr(fwts_framework > *fw, uint32_t attributes, uint > struct efi_setvariable setvariable; > uint64_t status; > uint64_t dataindex; > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -775,7 +771,6 @@ static int getnextvariable_multitest(fwts_framework > *fw, uint32_t multitesttime) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > > Acked-by: Alex Hung <alex.hung@canonical.com>
On 02/25/2013 05:40 PM, Colin Ian King wrote: > Fwd, we should pick this patch up. > > Colin > > -------- Original Message -------- > > X-Scanned-By: MailControl 13381.65 (www.mailcontrol.com) on 10.74.0.136 > > From: Matt Fleming <matt.fleming@intel.com> > > When calling GetVariable(), we need to fill out DataSize with the size > of the Data buffer instead of using whatever garbage value is left on > the stack. > > Furthermore, the Data buffers are not strings in any of the tests, so > there's no need to NUL-terminate them or make them 1-byte larger than > datasize, e.g. there's no real need to do this, > > uint8_t data[datasize+1; > > since the last element of data[] is never read back anyway. > > Cc: Ivan Hu <ivan.hu@canonical.com> > Cc: Colin Ian King <colin.king@canonical.com> > Cc: Alex Hung <alex.hung@canonical.com> > Signed-off-by: Matt Fleming <matt.fleming@intel.com> > --- > > Folks, is there a mailing list for the development of fwts? > > src/uefi/uefirtvariable/uefirtvariable.c | 19 +++++++------------ > 1 file changed, 7 insertions(+), 12 deletions(-) > > diff --git a/src/uefi/uefirtvariable/uefirtvariable.c > b/src/uefi/uefirtvariable/uefirtvariable.c > index e8aa041..e00b343 100644 > --- a/src/uefi/uefirtvariable/uefirtvariable.c > +++ b/src/uefi/uefirtvariable/uefirtvariable.c > @@ -89,15 +89,14 @@ static int getvariable_test(fwts_framework *fw, > uint64_t datasize, uint16_t *var > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > uint32_t i; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = >estguid1; > @@ -245,7 +244,6 @@ static int getnextvariable_test(fwts_framework *fw) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > @@ -350,11 +348,10 @@ static int > setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, u > uint64_t status; > uint64_t dataindex; > > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -392,9 +389,9 @@ static int setvariable_checkvariable(fwts_framework > *fw, uint64_t datasize, > struct efi_getvariable getvariable; > > uint64_t status; > - uint8_t testdata[datasize+1]; > + uint8_t testdata[datasize]; > uint64_t dataindex; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -442,7 +439,7 @@ static int > setvariable_checkvariable_notfound(fwts_framework *fw, uint16_t *varn > > uint64_t status; > uint8_t testdata[MAX_DATA_LENGTH]; > - uint64_t getdatasize; > + uint64_t getdatasize = sizeof(testdata); > uint32_t attributestest; > > getvariable.VariableName = varname; > @@ -472,11 +469,10 @@ static int setvariable_invalidattr(fwts_framework > *fw, uint32_t attributes, uint > struct efi_setvariable setvariable; > uint64_t status; > uint64_t dataindex; > - uint8_t data[datasize+1]; > + uint8_t data[datasize]; > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex + datadiff; > - data[dataindex] = '\0'; > > setvariable.VariableName = varname; > setvariable.VendorGuid = gtestguid; > @@ -775,7 +771,6 @@ static int getnextvariable_multitest(fwts_framework > *fw, uint32_t multitesttime) > > for (dataindex = 0; dataindex < datasize; dataindex++) > data[dataindex] = (uint8_t)dataindex; > - data[dataindex] = '\0'; > > setvariable.VariableName = variablenametest; > setvariable.VendorGuid = >estguid1; > > Tested with the patch, the behavior is correct. Thanks! Acked-by: Ivan Hu <ivan.hu@canonical.com>
diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c index e8aa041..e00b343 100644 --- a/src/uefi/uefirtvariable/uefirtvariable.c +++ b/src/uefi/uefirtvariable/uefirtvariable.c @@ -89,15 +89,14 @@ static int getvariable_test(fwts_framework *fw, uint64_t datasize, uint16_t *var uint64_t status; uint8_t testdata[MAX_DATA_LENGTH]; uint64_t dataindex; - uint64_t getdatasize; + uint64_t getdatasize = sizeof(testdata); uint32_t attributestest; - uint8_t data[datasize+1]; + uint8_t data[datasize]; uint32_t i; for (dataindex = 0; dataindex < datasize; dataindex++) data[dataindex] = (uint8_t)dataindex; - data[dataindex] = '\0'; setvariable.VariableName = varname;