Patchwork uefirtvariable: Correct DataSize for GetVariable()

login
register
mail settings
Submitter Colin King
Date Feb. 25, 2013, 9:40 a.m.
Message ID <512B31A0.3070907@canonical.com>
Download mbox | patch
Permalink /patch/222879/
State Accepted
Headers show

Comments

Colin King - Feb. 25, 2013, 9:40 a.m.
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(-)

  	setvariable.VendorGuid = &gtestguid1;
@@ -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 = &gtestguid1;
@@ -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 = &gtestguid1;
Colin King - Feb. 25, 2013, 11:45 a.m.
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 = &gtestguid1;
> @@ -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 = &gtestguid1;
> @@ -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 = &gtestguid1;

Acked-by: Colin Ian King <colin.king@canonical.com>
Alex Hung - Feb. 26, 2013, 1:06 a.m.
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 = &gtestguid1;
> @@ -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 = &gtestguid1;
> @@ -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 = &gtestguid1;
>
>
Acked-by: Alex Hung <alex.hung@canonical.com>
Ivan Hu - Feb. 26, 2013, 1:35 a.m.
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 = &gtestguid1;
> @@ -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 = &gtestguid1;
> @@ -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 = &gtestguid1;
>
>
Tested with the patch, the behavior is correct. Thanks!

Acked-by: Ivan Hu <ivan.hu@canonical.com>

Patch

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;