[PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Lee, Chun-Yi]

From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>

On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
named :

ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
/sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c

That causes by the following statement in efivar_create_sysfs_entry function:

 *(short_name + strlen(short_name)) = '-';
efi_guid_unparse(vendor_guid, short_name + strlen(short_name));

The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
as well for HP. So, the second strlen return the point of next '\0', causes there
have garbage string attached before GUID.

Tested on On HP z220.

Cc: Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>
Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Jeremy Kerr <jeremy.kerr-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Tested-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
---
 drivers/firmware/efivars.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 8bcb595..fbf18ff 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1708,7 +1708,7 @@ efivar_create_sysfs_entry(struct efivars *efivars,
 	/* This is ugly, but necessary to separate one vendor's
 	   private variables from another's.         */
 
-	*(short_name + strlen(short_name)) = '-';
+	strcpy(short_name + strlen(short_name), "-");
 	efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
 
 	new_efivar->kobj.kset = efivars->kset;
-- 
1.6.0.2

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Lingzhu Xiang]

On 03/01/2013 11:20 AM, Lee, Chun-Yi wrote:
> From: Michael Schroeder <mls@suse.com>
> 
> On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> named :
> 
> ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> That causes by the following statement in efivar_create_sysfs_entry function:
> 
>   *(short_name + strlen(short_name)) = '-';
> efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> 
> The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> as well for HP. So, the second strlen return the point of next '\0', causes there
> have garbage string attached before GUID.
> 
> Tested on On HP z220.

So short_name has trailing garbage, or rather, variable_name_size
is larger than variable_name actually is, wouldn't new_efivar->var.VariableName
also gets filled with trailing garbage?

In efivar_store_raw the VariableName's trailing garbage can cause
problem for comparison. You might want to also cover that or fix
variable_name_size.


Lingzhu Xiang

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 五，2013-03-01 於 17:31 +0800，Lingzhu Xiang 提到：
> On 03/01/2013 11:20 AM, Lee, Chun-Yi wrote:
> > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > 
> > On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> > named :
> > 
> > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > 
> > That causes by the following statement in efivar_create_sysfs_entry function:
> > 
> >   *(short_name + strlen(short_name)) = '-';
> > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > 
> > The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> > as well for HP. So, the second strlen return the point of next '\0', causes there
> > have garbage string attached before GUID.
> > 
> > Tested on On HP z220.
> 
> So short_name has trailing garbage, or rather, variable_name_size
> is larger than variable_name actually is, wouldn't new_efivar->var.VariableName
> also gets filled with trailing garbage?
> 
> In efivar_store_raw the VariableName's trailing garbage can cause
> problem for comparison. You might want to also cover that or fix
> variable_name_size.
> 
> 
> Lingzhu Xiang
> 

Yes, it's possible! I will add patch for print more debug messages to
bug reporter for check it.


Thanks for your review!
Joey Lee

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> 
> On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> named :
> 
> ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> That causes by the following statement in efivar_create_sysfs_entry function:
> 
>  *(short_name + strlen(short_name)) = '-';
> efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> 
> The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> as well for HP. So, the second strlen return the point of next '\0', causes there
> have garbage string attached before GUID.
> 
> Tested on On HP z220.

What's more likely happening here is that GetNextVariable() is broken on
this HP firmware and variable_name_size is too big for the given
variable in variable_name. We've seen other reports of similar bugs,

	https://bugzilla.kernel.org/show_bug.cgi?id=47631


Could someone try this patch against Linus' tree?

---

>From fe38d6f69c889b0f95c5548c724633aa58c2c99f Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Fri, 1 Mar 2013 14:49:12 +0000
Subject: [PATCH] efivars: Sanitise string length returned by
 GetNextVariableName()

Some buggy firmware implementations return a string length from
GetNextVariableName() that is actually larger than the string in
'variable_name', as Michael Schroeder writes,

  > On HP z220 system (firmware version 1.54), some EFI variables are
  > incorrectly named :
  >
  > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
  > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c

Since 'variable_name' is a string, we can validate its size by
searching for the terminating NULL character.

Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 35 ++++++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7320bf8..b5af292 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1653,6 +1653,33 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
 	return found;
 }
 
+/*
+ * Sanity check string length of a variable name.
+ */
+static unsigned long sanity_check_strlen(efi_char16_t *variable_name,
+					 unsigned long variable_name_size)
+{
+	efi_char16_t c;
+	unsigned long len;
+
+	/*
+	 * The variable name is, by definition, a NULL-terminated
+	 * string, so make absolutely sure that variable_name_size is
+	 * the value we expect it to be. If not, use whichever size is
+	 * smaller.
+	 */
+	for (len = 1; len <= variable_name_size; len++) {
+		c = variable_name[len - 1];
+		if (!c)
+			break;
+	}
+
+	if (len != variable_name_size)
+		printk(KERN_WARNING "efivars: bogus variable_name_size\n");
+
+	return min(len, variable_name_size);
+}
+
 static void efivar_update_sysfs_entries(struct work_struct *work)
 {
 	struct efivars *efivars = &__efivars;
@@ -1693,10 +1720,13 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
 		if (!found) {
 			kfree(variable_name);
 			break;
-		} else
+		} else {
+			variable_name_size = sanity_check_strlen(variable_name,
+								 variable_name_size);
 			efivar_create_sysfs_entry(efivars,
 						  variable_name_size,
 						  variable_name, &vendor);
+		}
 	}
 }
 
@@ -1941,8 +1971,11 @@ int register_efivars(struct efivars *efivars,
 		status = ops->get_next_variable(&variable_name_size,
 						variable_name,
 						&vendor_guid);
+
 		switch (status) {
 		case EFI_SUCCESS:
+			variable_name_size = sanity_check_strlen(variable_name,
+								 variable_name_size);
 			efivar_create_sysfs_entry(efivars,
 						  variable_name_size,
 						  variable_name,
-- 
1.7.11.7

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Fri, 2013-03-01 at 15:17 +0000, Matt Fleming wrote:
> On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > 
> > On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> > named :
> > 
> > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > 
> > That causes by the following statement in efivar_create_sysfs_entry function:
> > 
> >  *(short_name + strlen(short_name)) = '-';
> > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > 
> > The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> > as well for HP. So, the second strlen return the point of next '\0', causes there
> > have garbage string attached before GUID.
> > 
> > Tested on On HP z220.
> 
> What's more likely happening here is that GetNextVariable() is broken on
> this HP firmware and variable_name_size is too big for the given
> variable in variable_name. We've seen other reports of similar bugs,
> 
> 	https://bugzilla.kernel.org/show_bug.cgi?id=47631
> 
> 
> Could someone try this patch against Linus' tree?

Urgh, and here's a version that isn't utterly, utterly broken...

---

>From 4b2ef72bca72039717efe4570ec858a86d565b34 Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Fri, 1 Mar 2013 14:49:12 +0000
Subject: [PATCH] efivars: Sanitise string length returned by
 GetNextVariableName()

Some buggy firmware implementations return a string length from
GetNextVariableName() that is actually larger than the string in
'variable_name', as Michael Schroeder writes,

  > On HP z220 system (firmware version 1.54), some EFI variables are
  > incorrectly named :
  >
  > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
  > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c

Since 'variable_name' is a string, we can validate its size by
searching for the terminating NULL character.

Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7320bf8..ab477b8 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1895,6 +1895,33 @@ void unregister_efivars(struct efivars *efivars)
 }
 EXPORT_SYMBOL_GPL(unregister_efivars);
 
+/*
+ * Sanity check size of a variable name.
+ */
+static unsigned long sanity_check_size(efi_char16_t *variable_name,
+				       unsigned long variable_name_size)
+{
+	unsigned long len;
+	efi_char16_t c;
+
+	/*
+	 * The variable name is, by definition, a NULL-terminated
+	 * string, so make absolutely sure that variable_name_size is
+	 * the value we expect it to be. If not, return the real size.
+	 */
+	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
+		c = variable_name[(len / sizeof(c)) - 1];
+		if (!c)
+			break;
+	}
+
+
+	if (len != variable_name_size)
+		printk(KERN_WARNING "efivars: bogus variable_name_size: %lu %lu\n", len, variable_name_size);
+
+	return min(len, variable_name_size);
+}
+
 int register_efivars(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *parent_kobj)
@@ -1941,8 +1968,11 @@ int register_efivars(struct efivars *efivars,
 		status = ops->get_next_variable(&variable_name_size,
 						variable_name,
 						&vendor_guid);
+
 		switch (status) {
 		case EFI_SUCCESS:
+			variable_name_size = sanity_check_size(variable_name,
+							       variable_name_size);
 			efivar_create_sysfs_entry(efivars,
 						  variable_name_size,
 						  variable_name,
-- 
1.7.11.7

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 五，2013-03-01 於 16:31 +0000，Matt Fleming 提到：
> On Fri, 2013-03-01 at 15:17 +0000, Matt Fleming wrote:
> > On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> > > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > > 
> > > On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> > > named :
> > > 
> > > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > 
> > > That causes by the following statement in efivar_create_sysfs_entry function:
> > > 
> > >  *(short_name + strlen(short_name)) = '-';
> > > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > > 
> > > The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> > > as well for HP. So, the second strlen return the point of next '\0', causes there
> > > have garbage string attached before GUID.
> > > 
> > > Tested on On HP z220.
> > 
> > What's more likely happening here is that GetNextVariable() is broken on
> > this HP firmware and variable_name_size is too big for the given
> > variable in variable_name. We've seen other reports of similar bugs,
> > 
> > 	https://bugzilla.kernel.org/show_bug.cgi?id=47631
> > 
> > 
> > Could someone try this patch against Linus' tree?
> 
> Urgh, and here's a version that isn't utterly, utterly broken...

Thanks for Matt's patch, we will try it!

Joey Lee

> 
> ---
> 
> >From 4b2ef72bca72039717efe4570ec858a86d565b34 Mon Sep 17 00:00:00 2001
> From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Fri, 1 Mar 2013 14:49:12 +0000
> Subject: [PATCH] efivars: Sanitise string length returned by
>  GetNextVariableName()
> 
> Some buggy firmware implementations return a string length from
> GetNextVariableName() that is actually larger than the string in
> 'variable_name', as Michael Schroeder writes,
> 
>   > On HP z220 system (firmware version 1.54), some EFI variables are
>   > incorrectly named :
>   >
>   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
>   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
>   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> Since 'variable_name' is a string, we can validate its size by
> searching for the terminating NULL character.
> 
> Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
> Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/firmware/efivars.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7320bf8..ab477b8 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1895,6 +1895,33 @@ void unregister_efivars(struct efivars *efivars)
>  }
>  EXPORT_SYMBOL_GPL(unregister_efivars);
>  
> +/*
> + * Sanity check size of a variable name.
> + */
> +static unsigned long sanity_check_size(efi_char16_t *variable_name,
> +				       unsigned long variable_name_size)
> +{
> +	unsigned long len;
> +	efi_char16_t c;
> +
> +	/*
> +	 * The variable name is, by definition, a NULL-terminated
> +	 * string, so make absolutely sure that variable_name_size is
> +	 * the value we expect it to be. If not, return the real size.
> +	 */
> +	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
> +		c = variable_name[(len / sizeof(c)) - 1];
> +		if (!c)
> +			break;
> +	}
> +
> +
> +	if (len != variable_name_size)
> +		printk(KERN_WARNING "efivars: bogus variable_name_size: %lu %lu\n", len, variable_name_size);
> +
> +	return min(len, variable_name_size);
> +}
> +
>  int register_efivars(struct efivars *efivars,
>  		     const struct efivar_operations *ops,
>  		     struct kobject *parent_kobj)
> @@ -1941,8 +1968,11 @@ int register_efivars(struct efivars *efivars,
>  		status = ops->get_next_variable(&variable_name_size,
>  						variable_name,
>  						&vendor_guid);
> +
>  		switch (status) {
>  		case EFI_SUCCESS:
> +			variable_name_size = sanity_check_size(variable_name,
> +							       variable_name_size);
>  			efivar_create_sysfs_entry(efivars,
>  						  variable_name_size,
>  						  variable_name,

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

Hi Matt, 

於 六，2013-03-02 於 07:41 +0800，joeyli 提到：
> 於 五，2013-03-01 於 16:31 +0000，Matt Fleming 提到：
> > On Fri, 2013-03-01 at 15:17 +0000, Matt Fleming wrote:
> > > On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> > > > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > > > 
> > > > On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> > > > named :
> > > > 
> > > > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > 
> > > > That causes by the following statement in efivar_create_sysfs_entry function:
> > > > 
> > > >  *(short_name + strlen(short_name)) = '-';
> > > > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > > > 
> > > > The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> > > > as well for HP. So, the second strlen return the point of next '\0', causes there
> > > > have garbage string attached before GUID.
> > > > 
> > > > Tested on On HP z220.
> > > 
> > > What's more likely happening here is that GetNextVariable() is broken on
> > > this HP firmware and variable_name_size is too big for the given
> > > variable in variable_name. We've seen other reports of similar bugs,
> > > 
> > > 	https://bugzilla.kernel.org/show_bug.cgi?id=47631
> > > 
> > > 
> > > Could someone try this patch against Linus' tree?
> > 
> > Urgh, and here's a version that isn't utterly, utterly broken...
> 
> Thanks for Matt's patch, we will try it!
> 
> Joey Lee

Frederic confirmed your patch works to him for fix issue on HP machine.

Tested-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>

But, I suggest we just use the length with NULL in variable_name but not
VariableNameSize that was returned by GetNextVariableName.

My thinking is following...

> 
> > 
> > ---
> > 
> > >From 4b2ef72bca72039717efe4570ec858a86d565b34 Mon Sep 17 00:00:00 2001
> > From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Date: Fri, 1 Mar 2013 14:49:12 +0000
> > Subject: [PATCH] efivars: Sanitise string length returned by
> >  GetNextVariableName()
> > 
> > Some buggy firmware implementations return a string length from
> > GetNextVariableName() that is actually larger than the string in
> > 'variable_name', as Michael Schroeder writes,
> > 
> >   > On HP z220 system (firmware version 1.54), some EFI variables are
> >   > incorrectly named :
> >   >
> >   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> >   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> >   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > 
> > Since 'variable_name' is a string, we can validate its size by
> > searching for the terminating NULL character.
> > 
> > Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
> > Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> > Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> > Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/firmware/efivars.c | 30 ++++++++++++++++++++++++++++++
> >  1 file changed, 30 insertions(+)
> > 
> > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> > index 7320bf8..ab477b8 100644
> > --- a/drivers/firmware/efivars.c
> > +++ b/drivers/firmware/efivars.c
> > @@ -1895,6 +1895,33 @@ void unregister_efivars(struct efivars *efivars)
> >  }
> >  EXPORT_SYMBOL_GPL(unregister_efivars);
> >  
> > +/*
> > + * Sanity check size of a variable name.
> > + */
> > +static unsigned long sanity_check_size(efi_char16_t *variable_name,
> > +				       unsigned long variable_name_size)
> > +{
> > +	unsigned long len;
> > +	efi_char16_t c;
> > +
> > +	/*
> > +	 * The variable name is, by definition, a NULL-terminated
> > +	 * string, so make absolutely sure that variable_name_size is
> > +	 * the value we expect it to be. If not, return the real size.
> > +	 */
> > +	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
> > +		c = variable_name[(len / sizeof(c)) - 1];
> > +		if (!c)
> > +			break;
> > +	}
> > +

We just direct return the len here but don't need compare with
variable_name_size.

	return len;

> > +
> > +	if (len != variable_name_size)
> > +		printk(KERN_WARNING "efivars: bogus variable_name_size: %lu %lu\n", len, variable_name_size);
> > +
> > +	return min(len, variable_name_size);
> > +}
> > +

In UEFI 2.3.1 spec:

VariableNameSize		The size of the VariableName buffer

Note that if EFI_BUFFER_TOO_SMALL is returned, the VariableName buffer
was too small for the next variable. When such an error occurs, the
VariableNameSize is updated to reflect the size of buffer needed. In all
cases when calling GetNextVariableName() the VariableNameSize must not
exceed the actual buffer size that was allocated for VariableName.


Per spec, VariableNameSize will updated to 'the size of buffer needed'
when EFI_BUFFER_TOO_SMALL, but spec doesn't mention VariableNameSize
will also updated when EFI_SUCCESS. That means 'VariableNameSize = 1024'
when EFI_SUCCESS is not a bogus value.

> >  int register_efivars(struct efivars *efivars,
> >  		     const struct efivar_operations *ops,
> >  		     struct kobject *parent_kobj)
> > @@ -1941,8 +1968,11 @@ int register_efivars(struct efivars *efivars,
> >  		status = ops->get_next_variable(&variable_name_size,
> >  						variable_name,
> >  						&vendor_guid);
> > +
> >  		switch (status) {
> >  		case EFI_SUCCESS:
> > +			variable_name_size = sanity_check_size(variable_name,
> > +							       variable_name_size);
> >  			efivar_create_sysfs_entry(efivars,
> >  						  variable_name_size,
> >  						  variable_name,
> 

Due to variable_name_size could be 1024, so I suggest direct feed the
length of variable_name to efivar_create_sysfs_entry since we are need
to count the length.

This is a simply diff for my think:


diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 7320bf8..99a4f9f 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1895,6 +1895,21 @@ void unregister_efivars(struct efivars *efivars)
 }
 EXPORT_SYMBOL_GPL(unregister_efivars);
 
+static unsigned long variable_name_length(efi_char16_t *variable_name)
+{
+	unsigned long len;
+	efi_char16_t c;
+
+	len = 2;
+	do {
+		c = variable_name[len / sizeof(c) - 1];
+		if (c)
+			len += sizeof(c);
+	} while (c);
+
+	return len;
+}
+
 int register_efivars(struct efivars *efivars,
 		     const struct efivar_operations *ops,
 		     struct kobject *parent_kobj)
@@ -1944,7 +1959,7 @@ int register_efivars(struct efivars *efivars,
 		switch (status) {
 		case EFI_SUCCESS:
 			efivar_create_sysfs_entry(efivars,
-						  variable_name_size,
+						  variable_name_length(variable_name),
 						  variable_name,
 						  &vendor_guid);
 			break;

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 三，2013-03-06 於 15:34 +0800，joeyli 提到：
> Hi Matt, 
> 
> 於 六，2013-03-02 於 07:41 +0800，joeyli 提到：
> > 於 五，2013-03-01 於 16:31 +0000，Matt Fleming 提到：
> > > On Fri, 2013-03-01 at 15:17 +0000, Matt Fleming wrote:
> > > > On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> > > > > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > > > > 
> > > > > On HP z220 system (firmware version 1.54), some EFI variables
> are incorrectly
> > > > > named :
> > > > > 
> > > > > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > >
> > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >
> > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >
> > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > >
> > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > > 
> > > > > That causes by the following statement in
> efivar_create_sysfs_entry function:
> > > > > 
> > > > >  *(short_name + strlen(short_name)) = '-';
> > > > > efi_guid_unparse(vendor_guid, short_name +
> strlen(short_name));
> > > > > 
> > > > > The trailing \0 is overwritten with '-', but the next char
> doesn't seem to be a \0
> > > > > as well for HP. So, the second strlen return the point of next
> '\0', causes there
> > > > > have garbage string attached before GUID.
> > > > > 
> > > > > Tested on On HP z220.
> > > > 
> > > > What's more likely happening here is that GetNextVariable() is
> broken on
> > > > this HP firmware and variable_name_size is too big for the given
> > > > variable in variable_name. We've seen other reports of similar
> bugs,
> > > > 
> > > >   https://bugzilla.kernel.org/show_bug.cgi?id=47631
> > > > 
> > > > 
> > > > Could someone try this patch against Linus' tree?
> > > 
> > > Urgh, and here's a version that isn't utterly, utterly broken...
> > 
> > Thanks for Matt's patch, we will try it!
> > 
> > Joey Lee
> 
> Frederic confirmed your patch works to him for fix issue on HP
> machine.
> 
> Tested-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>

Sorry forgot paste variable_name_size from log:

efivars: bogus variable_name_size: 34 1024
efivars: bogus variable_name_size: 22 1024

The GetNextVariable() always return variable_name_size is 1024.


Thanks a lot!
Joey Lee

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Lingzhu Xiang]

On 03/06/2013 03:34 PM, joeyli wrote:
> +static unsigned long variable_name_length(efi_char16_t *variable_name)
> +{
> +	unsigned long len;
> +	efi_char16_t c;
> +
> +	len = 2;
> +	do {
> +		c = variable_name[len / sizeof(c) - 1];
> +		if (c)
> +			len += sizeof(c);
> +	} while (c);
> +
> +	return len;
> +}
> +

It looks like this function can be replaced with utf16_strsize.

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Wed, 2013-03-06 at 17:20 +0800, Lingzhu Xiang wrote:
> On 03/06/2013 03:34 PM, joeyli wrote:
> > +static unsigned long variable_name_length(efi_char16_t *variable_name)
> > +{
> > +	unsigned long len;
> > +	efi_char16_t c;
> > +
> > +	len = 2;
> > +	do {
> > +		c = variable_name[len / sizeof(c) - 1];
> > +		if (c)
> > +			len += sizeof(c);
> > +	} while (c);
> > +
> > +	return len;
> > +}
> > +
> 
> It looks like this function can be replaced with utf16_strsize.

Nearly. For this particular case we also need to count the terminating
NULL.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 三，2013-03-06 於 17:20 +0800，Lingzhu Xiang 提到：
> On 03/06/2013 03:34 PM, joeyli wrote:
> > +static unsigned long variable_name_length(efi_char16_t *variable_name)
> > +{
> > +	unsigned long len;
> > +	efi_char16_t c;
> > +
> > +	len = 2;
> > +	do {
> > +		c = variable_name[len / sizeof(c) - 1];
> > +		if (c)
> > +			len += sizeof(c);
> > +	} while (c);
> > +
> > +	return len;
> > +}
> > +
> 
> It looks like this function can be replaced with utf16_strsize.
> 

Ah! thanks for you point out, I will use utf16_strsize.


Thanks a lot!
Joey Lee

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Wed, 2013-03-06 at 15:34 +0800, joeyli wrote:
> Hi Matt, 
> 
> 於 六，2013-03-02 於 07:41 +0800，joeyli 提到：
> > 於 五，2013-03-01 於 16:31 +0000，Matt Fleming 提到：
> > > On Fri, 2013-03-01 at 15:17 +0000, Matt Fleming wrote:
> > > > On Fri, 2013-03-01 at 11:20 +0800, Lee, Chun-Yi wrote:
> > > > > From: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > > > > 
> > > > > On HP z220 system (firmware version 1.54), some EFI variables are incorrectly
> > > > > named :
> > > > > 
> > > > > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > > > > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > > > 
> > > > > That causes by the following statement in efivar_create_sysfs_entry function:
> > > > > 
> > > > >  *(short_name + strlen(short_name)) = '-';
> > > > > efi_guid_unparse(vendor_guid, short_name + strlen(short_name));
> > > > > 
> > > > > The trailing \0 is overwritten with '-', but the next char doesn't seem to be a \0
> > > > > as well for HP. So, the second strlen return the point of next '\0', causes there
> > > > > have garbage string attached before GUID.
> > > > > 
> > > > > Tested on On HP z220.
> > > > 
> > > > What's more likely happening here is that GetNextVariable() is broken on
> > > > this HP firmware and variable_name_size is too big for the given
> > > > variable in variable_name. We've seen other reports of similar bugs,
> > > > 
> > > > 	https://bugzilla.kernel.org/show_bug.cgi?id=47631
> > > > 
> > > > 
> > > > Could someone try this patch against Linus' tree?
> > > 
> > > Urgh, and here's a version that isn't utterly, utterly broken...
> > 
> > Thanks for Matt's patch, we will try it!
> > 
> > Joey Lee
> 
> Frederic confirmed your patch works to him for fix issue on HP machine.
> 
> Tested-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
> 
> But, I suggest we just use the length with NULL in variable_name but not
> VariableNameSize that was returned by GetNextVariableName.
> 
> My thinking is following...
> 
> > 
> > > 
> > > ---
> > > 
> > > >From 4b2ef72bca72039717efe4570ec858a86d565b34 Mon Sep 17 00:00:00 2001
> > > From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Date: Fri, 1 Mar 2013 14:49:12 +0000
> > > Subject: [PATCH] efivars: Sanitise string length returned by
> > >  GetNextVariableName()
> > > 
> > > Some buggy firmware implementations return a string length from
> > > GetNextVariableName() that is actually larger than the string in
> > > 'variable_name', as Michael Schroeder writes,
> > > 
> > >   > On HP z220 system (firmware version 1.54), some EFI variables are
> > >   > incorrectly named :
> > >   >
> > >   > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> > >   > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >   > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >   > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> > >   > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> > > 
> > > Since 'variable_name' is a string, we can validate its size by
> > > searching for the terminating NULL character.
> > > 
> > > Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
> > > Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
> > > Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
> > > Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
> > > Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > >  drivers/firmware/efivars.c | 30 ++++++++++++++++++++++++++++++
> > >  1 file changed, 30 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> > > index 7320bf8..ab477b8 100644
> > > --- a/drivers/firmware/efivars.c
> > > +++ b/drivers/firmware/efivars.c
> > > @@ -1895,6 +1895,33 @@ void unregister_efivars(struct efivars *efivars)
> > >  }
> > >  EXPORT_SYMBOL_GPL(unregister_efivars);
> > >  
> > > +/*
> > > + * Sanity check size of a variable name.
> > > + */
> > > +static unsigned long sanity_check_size(efi_char16_t *variable_name,
> > > +				       unsigned long variable_name_size)
> > > +{
> > > +	unsigned long len;
> > > +	efi_char16_t c;
> > > +
> > > +	/*
> > > +	 * The variable name is, by definition, a NULL-terminated
> > > +	 * string, so make absolutely sure that variable_name_size is
> > > +	 * the value we expect it to be. If not, return the real size.
> > > +	 */
> > > +	for (len = 2; len <= variable_name_size; len += sizeof(c)) {
> > > +		c = variable_name[(len / sizeof(c)) - 1];
> > > +		if (!c)
> > > +			break;
> > > +	}
> > > +
> 
> We just direct return the len here but don't need compare with
> variable_name_size.
> 
> 	return len;
> 
> > > +
> > > +	if (len != variable_name_size)
> > > +		printk(KERN_WARNING "efivars: bogus variable_name_size: %lu %lu\n", len, variable_name_size);
> > > +
> > > +	return min(len, variable_name_size);
> > > +}
> > > +
> 
> In UEFI 2.3.1 spec:
> 
> VariableNameSize		The size of the VariableName buffer
> 
> Note that if EFI_BUFFER_TOO_SMALL is returned, the VariableName buffer
> was too small for the next variable. When such an error occurs, the
> VariableNameSize is updated to reflect the size of buffer needed. In all
> cases when calling GetNextVariableName() the VariableNameSize must not
> exceed the actual buffer size that was allocated for VariableName.
> 
> 
> Per spec, VariableNameSize will updated to 'the size of buffer needed'
> when EFI_BUFFER_TOO_SMALL, but spec doesn't mention VariableNameSize
> will also updated when EFI_SUCCESS. That means 'VariableNameSize = 1024'
> when EFI_SUCCESS is not a bogus value.
> 
> > >  int register_efivars(struct efivars *efivars,
> > >  		     const struct efivar_operations *ops,
> > >  		     struct kobject *parent_kobj)
> > > @@ -1941,8 +1968,11 @@ int register_efivars(struct efivars *efivars,
> > >  		status = ops->get_next_variable(&variable_name_size,
> > >  						variable_name,
> > >  						&vendor_guid);
> > > +
> > >  		switch (status) {
> > >  		case EFI_SUCCESS:
> > > +			variable_name_size = sanity_check_size(variable_name,
> > > +							       variable_name_size);
> > >  			efivar_create_sysfs_entry(efivars,
> > >  						  variable_name_size,
> > >  						  variable_name,
> > 
> 
> Due to variable_name_size could be 1024, so I suggest direct feed the
> length of variable_name to efivar_create_sysfs_entry since we are need
> to count the length.
> 
> This is a simply diff for my think:
> 
> 
> diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
> index 7320bf8..99a4f9f 100644
> --- a/drivers/firmware/efivars.c
> +++ b/drivers/firmware/efivars.c
> @@ -1895,6 +1895,21 @@ void unregister_efivars(struct efivars *efivars)
>  }
>  EXPORT_SYMBOL_GPL(unregister_efivars);
>  
> +static unsigned long variable_name_length(efi_char16_t *variable_name)
> +{
> +	unsigned long len;
> +	efi_char16_t c;
> +
> +	len = 2;
> +	do {
> +		c = variable_name[len / sizeof(c) - 1];
> +		if (c)
> +			len += sizeof(c);
> +	} while (c);
> +
> +	return len;
> +}
> +
>  int register_efivars(struct efivars *efivars,
>  		     const struct efivar_operations *ops,
>  		     struct kobject *parent_kobj)
> @@ -1944,7 +1959,7 @@ int register_efivars(struct efivars *efivars,
>  		switch (status) {
>  		case EFI_SUCCESS:
>  			efivar_create_sysfs_entry(efivars,
> -						  variable_name_size,
> +						  variable_name_length(variable_name),
>  						  variable_name,
>  						  &vendor_guid);
>  			break;
> 

Hmm.. the reason I didn't implement the patch this way is because I do
think it's important to make sure we don't go out of bounds looking for
the terminating NULL, i.e. you need a 'len < variable_name_size' check
somewhere.

Care to update and resend your patch, ensuring we don't inspect more
than variable_name_size characters?

Also, which machine did you see this behaviour on?

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Frederic Crozat]

Le mercredi 06 mars 2013 à 11:19 +0000, Matt Fleming a écrit :

> Also, which machine did you see this behaviour on?

HP z220 desktop, System BIOS K51 v 1.54

this issue is only the "Secure Boot" related variables, which make me
think HP incorrectly created them in their latest firmware (where they
added Windows 8 support).

-- 
Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
SUSE

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Wed, 2013-03-06 at 12:58 +0100, Frederic Crozat wrote:
> Le mercredi 06 mars 2013 à 11:19 +0000, Matt Fleming a écrit :
> 
> > Also, which machine did you see this behaviour on?
> 
> HP z220 desktop, System BIOS K51 v 1.54
> 
> this issue is only the "Secure Boot" related variables, which make me
> think HP incorrectly created them in their latest firmware (where they
> added Windows 8 support).

Are you saying that the behaviour regarding VariableNameSize changes
depending on which variable is returned?

Mercy.....

Thanks for the info.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Frederic Crozat]

Le mercredi 06 mars 2013 à 12:36 +0000, Matt Fleming a écrit :
> On Wed, 2013-03-06 at 12:58 +0100, Frederic Crozat wrote:
> > Le mercredi 06 mars 2013 à 11:19 +0000, Matt Fleming a écrit :
> > 
> > > Also, which machine did you see this behaviour on?
> > 
> > HP z220 desktop, System BIOS K51 v 1.54
> > 
> > this issue is only the "Secure Boot" related variables, which make me
> > think HP incorrectly created them in their latest firmware (where they
> > added Windows 8 support).
> 
> Are you saying that the behaviour regarding VariableNameSize changes
> depending on which variable is returned?
>
> Mercy.....
 
Well, I only get the error message for some variables (8), not all of
them.

It was quite visible on unpatched kernel, with ls
-d /sys/firmware/efi/vars. 

ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
/sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/SignatureSupport-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
/sys/firmware/efi/vars/VendorKeys-pport8be4df61-93ca-11d2-aa0d-00e098032b8c

All other variable names were fine.
-- 
Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
SUSE

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Wed, 2013-03-06 at 14:30 +0100, Frederic Crozat wrote:
> Le mercredi 06 mars 2013 à 12:36 +0000, Matt Fleming a écrit :
> > On Wed, 2013-03-06 at 12:58 +0100, Frederic Crozat wrote:
> > > Le mercredi 06 mars 2013 à 11:19 +0000, Matt Fleming a écrit :
> > > 
> > > > Also, which machine did you see this behaviour on?
> > > 
> > > HP z220 desktop, System BIOS K51 v 1.54
> > > 
> > > this issue is only the "Secure Boot" related variables, which make me
> > > think HP incorrectly created them in their latest firmware (where they
> > > added Windows 8 support).
> > 
> > Are you saying that the behaviour regarding VariableNameSize changes
> > depending on which variable is returned?
> >
> > Mercy.....
>  
> Well, I only get the error message for some variables (8), not all of
> them.
> 
> It was quite visible on unpatched kernel, with ls
> -d /sys/firmware/efi/vars. 
> 
> ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
> /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/SignatureSupport-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> /sys/firmware/efi/vars/VendorKeys-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
> 
> All other variable names were fine.

Interesting. Thanks.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 三，2013-03-06 於 11:19 +0000，Matt Fleming 提到：
> On Wed, 2013-03-06 at 15:34 +0800, joeyli wrote:
...
> > +static unsigned long variable_name_length(efi_char16_t *variable_name)
> > +{
> > +	unsigned long len;
> > +	efi_char16_t c;
> > +
> > +	len = 2;
> > +	do {
> > +		c = variable_name[len / sizeof(c) - 1];
> > +		if (c)
> > +			len += sizeof(c);
> > +	} while (c);
> > +
> > +	return len;
> > +}
> > +
> >  int register_efivars(struct efivars *efivars,
> >  		     const struct efivar_operations *ops,
> >  		     struct kobject *parent_kobj)
> > @@ -1944,7 +1959,7 @@ int register_efivars(struct efivars *efivars,
> >  		switch (status) {
> >  		case EFI_SUCCESS:
> >  			efivar_create_sysfs_entry(efivars,
> > -						  variable_name_size,
> > +						  variable_name_length(variable_name),
> >  						  variable_name,
> >  						  &vendor_guid);
> >  			break;
> > 
> 
> Hmm.. the reason I didn't implement the patch this way is because I do
> think it's important to make sure we don't go out of bounds looking for
> the terminating NULL, i.e. you need a 'len < variable_name_size' check
> somewhere.
> 
> Care to update and resend your patch, ensuring we don't inspect more
> than variable_name_size characters?

The VariableNameSize is not reliable when EFI_SUCCESS is returned
because UEFI 2.3.1 spec only mention VariableNameSize should updated
when EFI_BUFFER_TOO_SMALL is returned. And, the 1024 bytes of buffer is
from old UEFI spec. There doesn't have any size condition of variable
data or variable name in 2.3.1 spec.

I modified the patch to grab VariableNameSize when EFI_BUFFER_TOO_SMALL,
the behavior like what we do in efivarfs_file_read().

This patch works on a normal UEFI machine, we will test it on HP z220. I
will send out it formally after test success.


Thanks a lot!
Joey Lee


>From 1f88fab2bdf07da51975d31c20ee66415c51e14e Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Date: Thu, 7 Mar 2013 18:14:25 +0800
Subject: [PATCH] efivars: Sanitise length of variable name for register

On HP z220 system (firmware version 1.54), some EFI variables have
incorrectly named :

 /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
 /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
 /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
 /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c

Matt Fleming and Lingzhu Xiang pointed out the reason is the variable_name_size
longer then the name string in variable_name when we feed them to
efivar_create_sysfs_entry().

Per UEFI 2.3.1 spec, the VariableNameSize is updated to reflect the size of buffer
that needed by VariableName when EFI_BUFFER_TOO_SMALL is returned. This behavior
is the same with the DataSize for GetVariable().

This patch do the same thing like efivarfs_file_read(), it feed a zero
VariableNameSize to GetNextVariableName() for grab the variable name size from
UEFI BIOS. When VariableNameSize larger then the buffer size of variable name, we
will reallocate more buffer to handle it.
The default buffer size is 1024, this number is from old UEFI spec.

In addition, we calculate the length of VariableName by using utf16_strsize()
but not direct feed VariableNameSize to efivar_create_sysfs_entry() because the
value of VariableNameSize is not reliable when EFI_SUCCESS is returned. UEFI spec
only claim the VariableNameSize updated on EFI_BUFFER_TOO_SMALL but not
on EFI_SUCCESS.

Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Cc: Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
---
 drivers/firmware/efivars.c |   35 +++++++++++++++++++++++++++--------
 1 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index f5596db..ff61f91 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1688,10 +1688,11 @@ int register_efivars(struct efivars *efivars,
 	efi_status_t status = EFI_NOT_FOUND;
 	efi_guid_t vendor_guid;
 	efi_char16_t *variable_name;
-	unsigned long variable_name_size = 1024;
+	unsigned long variable_name_size;
+	unsigned long variable_name_buff_size = 1024;
 	int error = 0;
 
-	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+	variable_name = kzalloc(variable_name_buff_size, GFP_KERNEL);
 	if (!variable_name) {
 		printk(KERN_ERR "efivars: Memory allocation failed.\n");
 		return -ENOMEM;
@@ -1722,17 +1723,35 @@ int register_efivars(struct efivars *efivars,
 	 */
 
 	do {
-		variable_name_size = 1024;
+		variable_name_size = 0;
 
 		status = ops->get_next_variable(&variable_name_size,
 						variable_name,
 						&vendor_guid);
 		switch (status) {
-		case EFI_SUCCESS:
-			efivar_create_sysfs_entry(efivars,
-						  variable_name_size,
-						  variable_name,
-						  &vendor_guid);
+		case EFI_BUFFER_TOO_SMALL:
+			if (variable_name_size < 2) {
+				/* set variable_name_size to buffer size when it's too small */
+				variable_name_size = variable_name_buff_size;
+			} else if (variable_name_size > variable_name_buff_size) {
+				/* re-allocate more buffer when size doesn't enough */
+				kfree(variable_name);
+				variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+				if (!variable_name) {
+					printk(KERN_ERR "efivars: Memory allocation failed.\n");
+					return -ENOMEM;
+				}
+				variable_name_buff_size = variable_name_size;
+			}
+			status = ops->get_next_variable(&variable_name_size,
+							variable_name,
+							&vendor_guid);
+			variable_name_size = utf16_strsize(variable_name, variable_name_buff_size)+2;
+			if (status == EFI_SUCCESS)
+				efivar_create_sysfs_entry(efivars,
+							  variable_name_size,
+							  variable_name,
+							  &vendor_guid);
 			break;
 		case EFI_NOT_FOUND:
 			break;
-- 
1.6.4.2

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Thu, 2013-03-07 at 18:34 +0800, joeyli wrote:
> The VariableNameSize is not reliable when EFI_SUCCESS is returned
> because UEFI 2.3.1 spec only mention VariableNameSize should updated
> when EFI_BUFFER_TOO_SMALL is returned. And, the 1024 bytes of buffer is
> from old UEFI spec. There doesn't have any size condition of variable
> data or variable name in 2.3.1 spec.

The spec may only mention what happens in the EFI_BUFFER_TOO_SMALL case,
but for EFI_SUCCESS, any behaviour other than leaving VariableNameSize
alone or updating it with the required size of the buffer is just
completely insane.

> I modified the patch to grab VariableNameSize when EFI_BUFFER_TOO_SMALL,
> the behavior like what we do in efivarfs_file_read().

Thanks, this does seem like the most robust solution.

> This patch works on a normal UEFI machine, we will test it on HP z220. I
> will send out it formally after test success.

Has anyone tried contacting HP to tell them their firmware is doing
bizarre things?

[...]

> @@ -1722,17 +1723,35 @@ int register_efivars(struct efivars *efivars,
>  	 */
>  
>  	do {
> -		variable_name_size = 1024;
> +		variable_name_size = 0;
>  
>  		status = ops->get_next_variable(&variable_name_size,
>  						variable_name,
>  						&vendor_guid);
>  		switch (status) {
> -		case EFI_SUCCESS:
> -			efivar_create_sysfs_entry(efivars,
> -						  variable_name_size,
> -						  variable_name,
> -						  &vendor_guid);
> +		case EFI_BUFFER_TOO_SMALL:
> +			if (variable_name_size < 2) {
> +				/* set variable_name_size to buffer size when it's too small */

This hunk would be better written like,

			if (variable_name_size < sizeof(efi_char16_t) * 2) {
				/* Bogus size - expect at least one char + NULL */
				variable_name_size = variable_name_buff_size;
			}

A variable name containing only '\0' is bogus. We need at least one
unicode character + '\0' for a valid variable name.

> +				variable_name_size = variable_name_buff_size;
> +			} else if (variable_name_size > variable_name_buff_size) {
> +				/* re-allocate more buffer when size doesn't enough */

This comment is redundant. Just delete it.

> +				kfree(variable_name);
> +				variable_name = kzalloc(variable_name_size, GFP_KERNEL);
> +				if (!variable_name) {
> +					printk(KERN_ERR "efivars: Memory allocation failed.\n");
> +					return -ENOMEM;
> +				}
> +				variable_name_buff_size = variable_name_size;
> +			}
> +			status = ops->get_next_variable(&variable_name_size,
> +							variable_name,
> +							&vendor_guid);
> +			variable_name_size = utf16_strsize(variable_name, variable_name_buff_size)+2;

Please document what this +2 represents and why we need it. Better yet,
use sizeof(efi_char16_t), and still document why it's needed, e.g. "Add
terminating NULL".

> +			if (status == EFI_SUCCESS)
> +				efivar_create_sysfs_entry(efivars,
> +							  variable_name_size,
> +							  variable_name,
> +							  &vendor_guid);
>  			break;
>  		case EFI_NOT_FOUND:
>  			break;


-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On Thu, 2013-03-07 at 11:39 +0000, Matt Fleming wrote:
> On Thu, 2013-03-07 at 18:34 +0800, joeyli wrote:
> > The VariableNameSize is not reliable when EFI_SUCCESS is returned
> > because UEFI 2.3.1 spec only mention VariableNameSize should updated
> > when EFI_BUFFER_TOO_SMALL is returned. And, the 1024 bytes of buffer is
> > from old UEFI spec. There doesn't have any size condition of variable
> > data or variable name in 2.3.1 spec.
> 
> The spec may only mention what happens in the EFI_BUFFER_TOO_SMALL case,
> but for EFI_SUCCESS, any behaviour other than leaving VariableNameSize
> alone or updating it with the required size of the buffer is just
> completely insane.
> 
> > I modified the patch to grab VariableNameSize when EFI_BUFFER_TOO_SMALL,
> > the behavior like what we do in efivarfs_file_read().
> 
> Thanks, this does seem like the most robust solution.

Also, you're probably going to need to update
efivar_update_sysfs_entries() too.

-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

Hi Matt, 

於 四，2013-03-07 於 13:57 +0000，Matt Fleming 提到：
> On Thu, 2013-03-07 at 11:39 +0000, Matt Fleming wrote:
> > On Thu, 2013-03-07 at 18:34 +0800, joeyli wrote:
> > > The VariableNameSize is not reliable when EFI_SUCCESS is returned
> > > because UEFI 2.3.1 spec only mention VariableNameSize should updated
> > > when EFI_BUFFER_TOO_SMALL is returned. And, the 1024 bytes of buffer is
> > > from old UEFI spec. There doesn't have any size condition of variable
> > > data or variable name in 2.3.1 spec.
> > 
> > The spec may only mention what happens in the EFI_BUFFER_TOO_SMALL case,
> > but for EFI_SUCCESS, any behaviour other than leaving VariableNameSize
> > alone or updating it with the required size of the buffer is just
> > completely insane.
> > 
> > > I modified the patch to grab VariableNameSize when EFI_BUFFER_TOO_SMALL,
> > > the behavior like what we do in efivarfs_file_read().
> > 
> > Thanks, this does seem like the most robust solution.
> 
> Also, you're probably going to need to update
> efivar_update_sysfs_entries() too.
> 

Thanks for your review! I will send out v2 patch after modify and test
on issue machine.


Thanks a lot!
Joey Lee

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

Hi Matt, 

於 四，2013-03-07 於 13:57 +0000，Matt Fleming 提到：
> On Thu, 2013-03-07 at 11:39 +0000, Matt Fleming wrote:
> > On Thu, 2013-03-07 at 18:34 +0800, joeyli wrote:
> > > The VariableNameSize is not reliable when EFI_SUCCESS is returned
> > > because UEFI 2.3.1 spec only mention VariableNameSize should updated
> > > when EFI_BUFFER_TOO_SMALL is returned. And, the 1024 bytes of buffer is
> > > from old UEFI spec. There doesn't have any size condition of variable
> > > data or variable name in 2.3.1 spec.
> > 
> > The spec may only mention what happens in the EFI_BUFFER_TOO_SMALL case,
> > but for EFI_SUCCESS, any behaviour other than leaving VariableNameSize
> > alone or updating it with the required size of the buffer is just
> > completely insane.
> > 
> > > I modified the patch to grab VariableNameSize when EFI_BUFFER_TOO_SMALL,
> > > the behavior like what we do in efivarfs_file_read().
> > 
> > Thanks, this does seem like the most robust solution.
> 
> Also, you're probably going to need to update
> efivar_update_sysfs_entries() too.
> 

Sorry for after I wrote patch, I think it's better we still use your
original patch to fix this bug, because I found the
efi_variable->VariableName allocated 1024 size and it also used by old
vars system. 

The following is my patch for reference, but I think your original patch
is better for backward compatible on variable name.

Please consider to merge your original patch!


Thanks a lot!
Joey Lee


>From c067288dbbb963b9cf9be4c5f59f5e39e88361ad Mon Sep 17 00:00:00 2001
From: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Date: Mon, 11 Mar 2013 18:26:04 +0800
Subject: [PATCH] efivars: Sanitise length of variable name for register


Signed-off-by: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
---
 drivers/firmware/efivars.c |   39 ++++++++++++++++++++++++++++++---------
 1 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 3edade0..1e854b3 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -111,7 +111,7 @@ MODULE_VERSION(EFIVARS_VERSION);
  */
 
 struct efi_variable {
-	efi_char16_t  VariableName[1024/sizeof(efi_char16_t)];
+	efi_char16_t  VariableName[1024/sizeof(efi_char16_t)];		/* PROBLEM: 1024 size, need backward compatible to old vars */
 	efi_guid_t    VendorGuid;
 	unsigned long DataSize;
 	__u8          Data[1024];
@@ -1903,10 +1903,11 @@ int register_efivars(struct efivars *efivars,
 	efi_status_t status = EFI_NOT_FOUND;
 	efi_guid_t vendor_guid;
 	efi_char16_t *variable_name;
-	unsigned long variable_name_size = 1024;
+	unsigned long variable_name_size;
+	unsigned long variable_name_buff_size = 1024;
 	int error = 0;
 
-	variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+	variable_name = kzalloc(variable_name_buff_size, GFP_KERNEL);
 	if (!variable_name) {
 		printk(KERN_ERR "efivars: Memory allocation failed.\n");
 		return -ENOMEM;
@@ -1937,17 +1938,37 @@ int register_efivars(struct efivars *efivars,
 	 */
 
 	do {
-		variable_name_size = 1024;
+		variable_name_size = 0;
 
 		status = ops->get_next_variable(&variable_name_size,
 						variable_name,
 						&vendor_guid);
 		switch (status) {
-		case EFI_SUCCESS:
-			efivar_create_sysfs_entry(efivars,
-						  variable_name_size,
-						  variable_name,
-						  &vendor_guid);
+		case EFI_BUFFER_TOO_SMALL:
+			if (variable_name_size < sizeof(efi_char16_t) * 2) {
+				/* Bogus size - expect at least one char + NULL */
+				variable_name_size = variable_name_buff_size;
+			} else if (variable_name_size > variable_name_buff_size) {
+				kfree(variable_name);
+				variable_name = kzalloc(variable_name_size, GFP_KERNEL);
+				if (!variable_name) {
+					printk(KERN_ERR "efivars: Memory allocation failed.\n");
+					return -ENOMEM;
+				}
+				variable_name_buff_size = variable_name_size;
+			}
+			status = ops->get_next_variable(&variable_name_size,
+							variable_name,
+							&vendor_guid);
+			/* Length of the variable_name, plus terminating NULL */
+			variable_name_size = utf16_strsize(
+					variable_name, variable_name_buff_size)
+					+ sizeof(efi_char16_t);
+			if (status == EFI_SUCCESS)
+				efivar_create_sysfs_entry(efivars,
+							  variable_name_size,	/* PROBLEM: variable_name_size could not larger then new_efivar->var.VariableName = 1024 */
+							  variable_name,
+							  &vendor_guid);
 			break;
 		case EFI_NOT_FOUND:
 			break;
-- 
1.6.4.2

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[Matt Fleming]

On 03/11/2013 01:17 PM, joeyli wrote:
> Sorry for after I wrote patch, I think it's better we still use your
> original patch to fix this bug, because I found the
> efi_variable->VariableName allocated 1024 size and it also used by old
> vars system. 
> 
> The following is my patch for reference, but I think your original patch
> is better for backward compatible on variable name.
> 
> Please consider to merge your original patch!

OK, this is what I've got queued up (note I removed the warning).

---

>From afa9ae7bf47145d661487f88f2ec67b062ca98bc Mon Sep 17 00:00:00 2001
From: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Date: Fri, 1 Mar 2013 14:49:12 +0000
Subject: [PATCH] efivars: explicitly calculate length of VariableName

It's not wise to assume VariableNameSize represents the length of
VariableName, as not all firmware updates VariableNameSize in the same
way (some don't update it at all if EFI_SUCCESS is returned). There
are even implementations out there that update VariableNameSize with
values that are both larger than the string returned in VariableName
and smaller than the buffer passed to GetNextVariableName(), which
resulted in the following bug report from Michael Schroeder,

  > On HP z220 system (firmware version 1.54), some EFI variables are
  > incorrectly named :
  >
  > ls -d /sys/firmware/efi/vars/*8be4d* | grep -v -- -8be returns
  > /sys/firmware/efi/vars/dbxDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/KEKDefault-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SecureBoot-pport8be4df61-93ca-11d2-aa0d-00e098032b8c
  > /sys/firmware/efi/vars/SetupMode-Information8be4df61-93ca-11d2-aa0d-00e098032b8c
The issue here is that because we blindly use VariableNameSize without
verifying its value, we can potentially read garbage values from the
buffer containing VariableName if VariableNameSize is larger than the
length of VariableName.

Since VariableName is a string, we can calculate its size by searching
for the terminating NULL character.

Reported-by: Frederic Crozat <fcrozat-IBi9RG/b67k@public.gmane.org>
Cc: Matthew Garrett <mjg59-1xO5oi07KQx4cg9Nei1l7Q@public.gmane.org>
Cc: Josh Boyer <jwboyer-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Michael Schroeder <mls-IBi9RG/b67k@public.gmane.org>
Cc: Lee, Chun-Yi <jlee-IBi9RG/b67k@public.gmane.org>
Cc: Lingzhu Xiang <lxiang-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: Seiji Aguchi <seiji.aguchi-7rDLJAbr9SE@public.gmane.org>
Signed-off-by: Matt Fleming <matt.fleming-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
 drivers/firmware/efivars.c | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index d90b061..ae26d5e 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -1704,6 +1704,31 @@ static bool variable_is_present(efi_char16_t *variable_name, efi_guid_t *vendor)
        return found;
 }
 
+/*
+ * Returns the size of variable_name, in bytes, including the
+ * terminating NULL character, or variable_name_size if no NULL
+ * character is found among the first variable_name_size bytes.
+ */
+static unsigned long var_name_strnsize(efi_char16_t *variable_name,
+                                      unsigned long variable_name_size)
+{
+       unsigned long len;
+       efi_char16_t c;
+
+       /*
+        * The variable name is, by definition, a NULL-terminated
+        * string, so make absolutely sure that variable_name_size is
+        * the value we expect it to be. If not, return the real size.
+        */
+       for (len = 2; len <= variable_name_size; len += sizeof(c)) {
+               c = variable_name[(len / sizeof(c)) - 1];
+               if (!c)
+                       break;
+       }
+
+       return min(len, variable_name_size);
+}
+
 static void efivar_update_sysfs_entries(struct work_struct *work)
 {
        struct efivars *efivars = &__efivars;
@@ -1744,10 +1769,13 @@ static void efivar_update_sysfs_entries(struct work_struct *work)
                if (!found) {
                        kfree(variable_name);
                        break;
-               } else
+               } else {
+                       variable_name_size = var_name_strnsize(variable_name,
+                                                              variable_name_size);
                        efivar_create_sysfs_entry(efivars,
                                                  variable_name_size,
                                                  variable_name, &vendor);
+               }
        }
 }
 
@@ -1994,6 +2022,8 @@ int register_efivars(struct efivars *efivars,
                                                &vendor_guid);
                switch (status) {
                case EFI_SUCCESS:
+                       variable_name_size = var_name_strnsize(variable_name,
+                                                              variable_name_size);
                        efivar_create_sysfs_entry(efivars,
                                                  variable_name_size,
                                                  variable_name,
-- 
1.7.11.7



-- 
Matt Fleming, Intel Open Source Technology Center

Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using strcpy to replace null with dash

[joeyli]

於 四，2013-03-07 於 11:39 +0000，Matt Fleming 提到：
> > This patch works on a normal UEFI machine, we will test it on HP
> z220. I
> > will send out it formally after test success.
> 
> Has anyone tried contacting HP to tell them their firmware is doing
> bizarre things?

We will try to contact with HP workstation department.
Another good chance is in UEFI Plugfest Taipei at next next week, I will
discuss with HP engineer for this issue.


Thanks a lot!
Joey Lee