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

[joeyli <jlee-IBi9RG/b67k@public.gmane.org>]

於 三，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