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@xxxxxxxx> > > > > > > > > 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@xxxxxxxx> 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@xxxxxxxxx> > > 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@xxxxxxxx> > > Cc: Matthew Garrett <mjg59@xxxxxxxxxxxxx> > > Cc: Josh Boyer <jwboyer@xxxxxxxxxx> > > Cc: Michael Schroeder <mls@xxxxxxxx> > > Cc: Lee, Chun-Yi <jlee@xxxxxxxx> > > Cc: Lingzhu Xiang <lxiang@xxxxxxxxxx> > > Signed-off-by: Matt Fleming <matt.fleming@xxxxxxxxx> > > --- > > 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; -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
