[PATCH] EFI missing failure handling

[PATCH] EFI missing failure handling

[Panagiotis Issaris]

Hi,

The EFI driver allocates memory and writes into it without checking the
success of the allocation:

668     efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
...
696     memset(variable_name, 0, 1024);

The patch applies to 2.6.11-bk1.

Signed-off-by: Panagiotis Issaris <panagiotis.issaris@mech.kuleuven.ac.be>

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 02:23:04.000000000 +0100
@@ -670,6 +670,9 @@ efivars_init(void)
 	unsigned long variable_name_size = 1024;
 	int i, rc = 0, error = 0;
 
+	if (!variable_name)
+		return -ENOMEM;
+
 	if (!efi_enabled)
 		return -ENODEV;
 
-- 
  K.U.Leuven, Mechanical Eng.,  Mechatronics & Robotics Research Group
  http://people.mech.kuleuven.ac.be/~pissaris/

Re: [PATCH] EFI missing failure handling

[Alexey Dobriyan]

On Saturday 05 March 2005 17:38, Panagiotis Issaris wrote:

> The EFI driver allocates memory and writes into it without checking the
> success of the allocation:
> 
> 668     efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
> ...
> 696     memset(variable_name, 0, 1024);

> --- linux-2.6.11-orig/drivers/firmware/efivars.c
> +++ linux-2.6.11-pi/drivers/firmware/efivars.c
> @@ -670,6 +670,9 @@ efivars_init(void)

> +	if (!variable_name)
> +		return -ENOMEM;
> +
>  	if (!efi_enabled)
>  		return -ENODEV; 

I'd better move kmalloc() and checking for success down right before
memset(). Otherwise you leak if efi_enabled == 0.

Oh, and efivars_init() wants to return "error", not unconditionally 0.

	Alexey

Re: [PATCH] EFI missing failure handling

[Panagiotis Issaris]

Hi,

On Sat, Mar 05, 2005 at 07:06:29PM +0200 or thereabouts, Alexey Dobriyan wrote:
> On Saturday 05 March 2005 17:38, Panagiotis Issaris wrote:
> 
> > The EFI driver allocates memory and writes into it without checking the
> > success of the allocation:
> > 
> > 668     efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
> > ...
> > 696     memset(variable_name, 0, 1024);
> 
> > --- linux-2.6.11-orig/drivers/firmware/efivars.c
> > +++ linux-2.6.11-pi/drivers/firmware/efivars.c
> > @@ -670,6 +670,9 @@ efivars_init(void)
> 
> > +	if (!variable_name)
> > +		return -ENOMEM;
> > +
> >  	if (!efi_enabled)
> >  		return -ENODEV; 
> 
> I'd better move kmalloc() and checking for success down right before
> memset(). Otherwise you leak if efi_enabled == 0.
> Oh, and efivars_init() wants to return "error", not unconditionally 0.
> 
> 	Alexey

Thanks! How about the updated patch?

With friendly regards,
Takis

Signed-off-by: Panagiotis Issaris <panagiotis.issaris@mech.kuleuven.ac.be>

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

-- 
  K.U.Leuven, Mechanical Eng.,  Mechatronics & Robotics Research Group
  http://people.mech.kuleuven.ac.be/~pissaris/

Re: [PATCH] EFI missing failure handling

[Matt Domsch]

On Sat, Mar 05, 2005 at 09:17:34PM +0100, Panagiotis Issaris wrote:
> Hi,
> 
> On Sat, Mar 05, 2005 at 07:06:29PM +0200 or thereabouts, Alexey Dobriyan wrote:
> > On Saturday 05 March 2005 17:38, Panagiotis Issaris wrote:
> > 
> > > The EFI driver allocates memory and writes into it without checking the
> > > success of the allocation:
> > > 
> > > 668     efi_char16_t *variable_name = kmalloc(1024, GFP_KERNEL);
> > > ...
> > > 696     memset(variable_name, 0, 1024);
> > 
> > > --- linux-2.6.11-orig/drivers/firmware/efivars.c
> > > +++ linux-2.6.11-pi/drivers/firmware/efivars.c
> > > @@ -670,6 +670,9 @@ efivars_init(void)
> > 
> > > +	if (!variable_name)
> > > +		return -ENOMEM;
> > > +
> > >  	if (!efi_enabled)
> > >  		return -ENODEV; 
> > 
> > I'd better move kmalloc() and checking for success down right before
> > memset(). Otherwise you leak if efi_enabled == 0.
> > Oh, and efivars_init() wants to return "error", not unconditionally 0.
> > 
> > 	Alexey
> 
> Thanks! How about the updated patch?

Looks good to me.  Good catch, and thanks for the patch!  Please
forward to Andrew Morton (akpm@osdl.org) directly, following the
format described here: http://linux.yyz.us/patch-format.html

Thanks,
Matt

-- 
Matt Domsch
Software Architect
Dell Linux Solutions linux.dell.com & www.dell.com/linux
Linux on Dell mailing lists @ http://lists.us.dell.com