| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 11 Mar 2013 21:17:53 +0800
From: joeyli <jlee@...e.com>
To: Matt Fleming <matt@...sole-pimps.org>
Cc: linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
Michael Schroeder <mls@...e.com>,
Josh Boyer <jwboyer@...hat.com>,
Peter Jones <pjones@...hat.com>,
Matthew Garrett <mjg59@...f.ucam.org>,
Frederic Crozat <fcrozat@...e.com>
Subject: Re: [PATCH] efivarfs: fix abnormal GUID in variable name by using
strcpy to replace null with dash
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@...e.com>
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@...e.com>
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists