Patchwork [1/3] uefirtvariable: Check new VariableNameSize from GetNextVariableName()

login
register
mail settings
Submitter Matt Fleming
Date March 5, 2013, 9:54 p.m.
Message ID <1362520494-8917-2-git-send-email-matt@console-pimps.org>
Download mbox | patch
Permalink /patch/225185/
State Rejected
Headers show

Comments

Matt Fleming - March 5, 2013, 9:54 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 do update VariableNameSize
with the value of strlen(VariableName) + 1, so print a warning if a
different value is found.

Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
 src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
 1 file changed, 72 insertions(+), 2 deletions(-)
Colin King - March 5, 2013, 10:15 p.m.
On 05/03/13 21:54, 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 do update VariableNameSize
> with the value of strlen(VariableName) + 1, so print a warning if a
> different value is found.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>   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..177dbf9 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 (!strlen_valid(getnextvariablename.VariableName,
> +				  *getnextvariablename.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: Colin Ian King <colin.king@canonical.com>
Ivan Hu - March 6, 2013, 3:08 a.m.
On 03/06/2013 05:54 AM, 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 do update VariableNameSize
> with the value of strlen(VariableName) + 1, so print a warning if a
> different value is found.
>
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>   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..177dbf9 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 (!strlen_valid(getnextvariablename.VariableName,
> +				  *getnextvariablename.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>
Matt Fleming - March 6, 2013, 11:27 a.m.
On Tue, 2013-03-05 at 21:54 +0000, 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 do update VariableNameSize
> with the value of strlen(VariableName) + 1, so print a warning if a
> different value is found.
> 
> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> ---
>  src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
>  1 file changed, 72 insertions(+), 2 deletions(-)

Folks, I was fairly certain that there were no implementations in the
wild that failed to update VariableNameSize on EFI_SUCCESS, but I've
just been informed that there are some.

You may or may not want to take this patch, since it is warning about an
undocumented behaviour - albeit one that many implementations exhibit.
Colin King - March 6, 2013, 11:32 a.m.
On 06/03/13 11:27, Matt Fleming wrote:
> On Tue, 2013-03-05 at 21:54 +0000, 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 do update VariableNameSize
>> with the value of strlen(VariableName) + 1, so print a warning if a
>> different value is found.
>>
>> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
>> ---
>>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
>>   1 file changed, 72 insertions(+), 2 deletions(-)
>
> Folks, I was fairly certain that there were no implementations in the
> wild that failed to update VariableNameSize on EFI_SUCCESS, but I've
> just been informed that there are some.

Urgh, is this because the spec is a tad ambiguous that we're seeing this 
in the wild?

>
> You may or may not want to take this patch, since it is warning about an
> undocumented behaviour - albeit one that many implementations exhibit.
>

One of the remits of fwts is to catch ambiguous behaviour so firmware 
can be fixed before it gets released.  Will catching this kind of 
behaviour be useful? If so, there seems merit to keeping it.

Colin
Matt Fleming - March 6, 2013, 12:08 p.m.
On Wed, 2013-03-06 at 11:32 +0000, Colin Ian King wrote:
> On 06/03/13 11:27, Matt Fleming wrote:
> > On Tue, 2013-03-05 at 21:54 +0000, 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 do update VariableNameSize
> >> with the value of strlen(VariableName) + 1, so print a warning if a
> >> different value is found.
> >>
> >> Signed-off-by: Matt Fleming <matt.fleming@intel.com>
> >> ---
> >>   src/uefi/uefirtvariable/uefirtvariable.c | 74 +++++++++++++++++++++++++++++++-
> >>   1 file changed, 72 insertions(+), 2 deletions(-)
> >
> > Folks, I was fairly certain that there were no implementations in the
> > wild that failed to update VariableNameSize on EFI_SUCCESS, but I've
> > just been informed that there are some.
> 
> Urgh, is this because the spec is a tad ambiguous that we're seeing this 
> in the wild?

Yeah. The GetNextVariableName() documentation in Section 7.2 of the UEFI
2.3.1 spec doesn't say what should happen to VariableNameSize in the
case of EFI_SUCCESS. It only says that it should be updated to the size
of the buffer required to hold VariableName in the EFI_BUFFER_TOO_SMALL
case.

But it's worth pointing out that upstream EDK2 does update
VariableNameSize with the size of VariableName on every invocation,
which means it's likely that firmware that also has this behaviour is
based on EDK2 or some derivative. Any implementation that doesn't is
probably of the older, weirder variety.
 
> >
> > You may or may not want to take this patch, since it is warning about an
> > undocumented behaviour - albeit one that many implementations exhibit.
> >
> 
> One of the remits of fwts is to catch ambiguous behaviour so firmware 
> can be fixed before it gets released.  Will catching this kind of 
> behaviour be useful? If so, there seems merit to keeping it.

The bug that this test was designed to catch is one affecting HP
machines where VariableNameSize is updated on every invocation, but the
updated value is neither the size of VariableName nor the initial value
of VaraibleNameSize - it's some value in the middle and I'm not quite
sure what it is supposed to represent.

I think there's merit in catching that particular issue, but probably
not in warning if VariableNameSize isn't updated at all. I'll send a v2
of this patch with those changes.

Patch

diff --git a/src/uefi/uefirtvariable/uefirtvariable.c b/src/uefi/uefirtvariable/uefirtvariable.c
index e00b343..177dbf9 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 (!strlen_valid(getnextvariablename.VariableName,
+				  *getnextvariablename.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;