The EFI driver allocates memory and writes into it without checking the
success of the allocation. Furthermore, on failure of the firmware_register() it
doesn't free the allocated memory and on failure of the subsys_create_file()
calls it returns zero instead of the errorcode.
Signed-off-by: Panagiotis Issaris <[email protected]>
diff -pruN linux-2.6.11-orig/drivers/firmware/efivars.c linux-2.6.11-pi/drivers/firmware/efivars.c
--- linux-2.6.11-orig/drivers/firmware/efivars.c 2005-03-05 02:23:29.000000000 +0100
+++ linux-2.6.11-pi/drivers/firmware/efivars.c 2005-03-05 21:09:33.000000000 +0100
@@ -665,13 +665,19 @@ efivars_init(void)
{
efi_status_t status = EFI_NOT_FOUND;
efi_guid_t vendor_guid;
- efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
+ efi_char16_t *variable_name;
struct subsys_attribute *attr;
unsigned long variable_name_size = 1024;
int i, rc = 0, error = 0;
if (!efi_enabled)
return -ENODEV;
+
+ variable_name = kmalloc(variable_name_size, GFP_KERNEL);
+ if (!variable_name)
+ return -ENOMEM;
+
+ memset(variable_name, 0, variable_name_size);
printk(KERN_INFO "EFI Variables Facility v%s %s\n", EFIVARS_VERSION,
EFIVARS_DATE);
@@ -682,8 +688,10 @@ efivars_init(void)
rc = firmware_register(&efi_subsys);
- if (rc)
+ if (rc) {
+ kfree(variable_name);
return rc;
+ }
kset_set_kset_s(&vars_subsys, efi_subsys);
subsystem_register(&vars_subsys);
@@ -693,8 +701,6 @@ efivars_init(void)
* the variable name and variable data is 1024 bytes.
*/
- memset(variable_name, 0, 1024);
-
do {
variable_name_size = 1024;
@@ -735,7 +741,7 @@ efivars_init(void)
}
kfree(variable_name);
- return 0;
+ return error;
}
static void __exit
"Panagiotis Issaris" <[email protected]> wrote:
>
> The EFI driver allocates memory and writes into it without checking the
> success of the allocation. Furthermore, on failure of the firmware_register() it
> doesn't free the allocated memory and on failure of the subsys_create_file()
> calls it returns zero instead of the errorcode.
>
Fair enough. But there's potential for behavioural change here.
If subsys_create_file() returns an error then efivars_init() will now
propagate that error. I suspect we'll need a firmware_unregister() in that
case.
>
> diff -pruN linux-2.6.11-orig/drivers/firmware/efivars.c linux-2.6.11-pi/drivers/firmware/efivars.c
> --- linux-2.6.11-orig/drivers/firmware/efivars.c 2005-03-05 02:23:29.000000000 +0100
> +++ linux-2.6.11-pi/drivers/firmware/efivars.c 2005-03-05 21:09:33.000000000 +0100
> @@ -665,13 +665,19 @@ efivars_init(void)
> {
> efi_status_t status = EFI_NOT_FOUND;
> efi_guid_t vendor_guid;
> - efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
> + efi_char16_t *variable_name;
> struct subsys_attribute *attr;
> unsigned long variable_name_size = 1024;
> int i, rc = 0, error = 0;
>
> if (!efi_enabled)
> return -ENODEV;
> +
> + variable_name = kmalloc(variable_name_size, GFP_KERNEL);
> + if (!variable_name)
> + return -ENOMEM;
> +
> + memset(variable_name, 0, variable_name_size);
>
> printk(KERN_INFO "EFI Variables Facility v%s %s\n", EFIVARS_VERSION,
> EFIVARS_DATE);
> @@ -682,8 +688,10 @@ efivars_init(void)
>
> rc = firmware_register(&efi_subsys);
>
> - if (rc)
> + if (rc) {
> + kfree(variable_name);
> return rc;
> + }
>
> kset_set_kset_s(&vars_subsys, efi_subsys);
> subsystem_register(&vars_subsys);
> @@ -693,8 +701,6 @@ efivars_init(void)
> * the variable name and variable data is 1024 bytes.
> */
>
> - memset(variable_name, 0, 1024);
> -
> do {
> variable_name_size = 1024;
>
> @@ -735,7 +741,7 @@ efivars_init(void)
> }
>
> kfree(variable_name);
> - return 0;
> + return error;
> }
>
> static void __exit
Hi,
On Sun, Mar 06, 2005 at 02:06:27AM -0800 or thereabouts, Andrew Morton wrote:
> "Panagiotis Issaris" <[email protected]> wrote:
> >
> > The EFI driver allocates memory and writes into it without checking the
> > success of the allocation. Furthermore, on failure of the firmware_register() it
> > doesn't free the allocated memory and on failure of the subsys_create_file()
> > calls it returns zero instead of the errorcode.
> >
>
> Fair enough. But there's potential for behavioural change here.
>
> If subsys_create_file() returns an error then efivars_init() will now
> propagate that error. I suspect we'll need a firmware_unregister() in that
> case.
You're right. Additionally, possible failure of subsystem_register() wasn't
handled. I've "restyled" the failure handling a bit, to be more like the
generally used failure handling code. The last error check handling check feels
a bit strange to me. Should I try to get it a bit clearer?
Should I update the changelog, version and date which are embedded in the driver?
diff -pruN linux-2.6.11-orig/drivers/firmware/efivars.c linux-2.6.11-pi/drivers/firmware/efivars.c
--- linux-2.6.11-orig/drivers/firmware/efivars.c 2005-03-05 02:23:29.000000000 +0100
+++ linux-2.6.11-pi/drivers/firmware/efivars.c 2005-03-06 12:23:41.000000000 +0100
@@ -665,13 +665,21 @@ efivars_init(void)
{
efi_status_t status = EFI_NOT_FOUND;
efi_guid_t vendor_guid;
- efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
+ efi_char16_t *variable_name;
struct subsys_attribute *attr;
unsigned long variable_name_size = 1024;
- int i, rc = 0, error = 0;
+ int i, error = 0;
if (!efi_enabled)
return -ENODEV;
+
+ variable_name = kmalloc(variable_name_size, GFP_KERNEL);
+ if (!variable_name) {
+ printk(KERN_ERR "efivars: Memory allocation failed.\n");
+ return -ENOMEM;
+ }
+
+ memset(variable_name, 0, variable_name_size);
printk(KERN_INFO "EFI Variables Facility v%s %s\n", EFIVARS_VERSION,
EFIVARS_DATE);
@@ -680,21 +688,27 @@ efivars_init(void)
* For now we'll register the efi subsys within this driver
*/
- rc = firmware_register(&efi_subsys);
+ error = firmware_register(&efi_subsys);
- if (rc)
- return rc;
+ if (error) {
+ printk(KERN_ERR "efivars: Firmware registration failed with error %d.\n", error);
+ goto out_free;
+ }
kset_set_kset_s(&vars_subsys, efi_subsys);
- subsystem_register(&vars_subsys);
+
+ error = subsystem_register(&vars_subsys);
+
+ if (error) {
+ printk(KERN_ERR "efivars: Subsystem registration failed with error %d.\n", error);
+ goto out_firmware_unregister;
+ }
/*
* Per EFI spec, the maximum storage allocated for both
* the variable name and variable data is 1024 bytes.
*/
- memset(variable_name, 0, 1024);
-
do {
variable_name_size = 1024;
@@ -734,8 +748,20 @@ efivars_init(void)
error = subsys_create_file(&efi_subsys, attr);
}
+ if (error)
+ printk(KERN_ERR "efivars: Sysfs attribute export failed with error %d.\n", error);
+ else
+ goto out_free;
+
+ subsystem_unregister(&vars_subsys);
+
+out_firmware_unregister:
+ firmware_unregister(&efi_subsys);
+
+out_free:
kfree(variable_name);
- return 0;
+
+ return error;
}
static void __exit