Browse Source

Input: serio HIL MLC - don't deref null, don't leak and return proper error

While reviewing various users of kernel memory allocation functions I came
across drivers/input/serio/hil_mlc.c::hil_mlc_register() and noticed that:

 - it calls kzalloc() but fails to check for a NULL return before use.
 - it makes several allocations and if one fails it doesn't free the
   previous ones.
 - It doesn't return -ENOMEM in the failed memory allocation case (it just
   crashes).

This patch corrects all of the above and also reworks the only caller of
this function that I could find
(drivers/input/serio/hp_sdc_mlc.c::hp_sdc_mlc_out()) so that it now checks
the return value of hil_mlc_register() and properly propagates it on
failure and I also restructured the code to remove some labels and goto's
to make it, IMHO nicer to read.

Signed-off-by: Jesper Juhl <jj@chaosbits.net>
Tested-by: Helge Deller <deller@gmx.de>
Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
Jesper Juhl 14 years ago
parent
commit
39de52104d
2 changed files with 14 additions and 9 deletions
  1. 5 0
      drivers/input/serio/hil_mlc.c
  2. 9 9
      drivers/input/serio/hp_sdc_mlc.c

+ 5 - 0
drivers/input/serio/hil_mlc.c

@@ -932,6 +932,11 @@ int hil_mlc_register(hil_mlc *mlc)
 		hil_mlc_copy_di_scratch(mlc, i);
 		hil_mlc_copy_di_scratch(mlc, i);
 		mlc_serio = kzalloc(sizeof(*mlc_serio), GFP_KERNEL);
 		mlc_serio = kzalloc(sizeof(*mlc_serio), GFP_KERNEL);
 		mlc->serio[i] = mlc_serio;
 		mlc->serio[i] = mlc_serio;
+		if (!mlc->serio[i]) {
+			for (; i >= 0; i--)
+				kfree(mlc->serio[i]);
+			return -ENOMEM;
+		}
 		snprintf(mlc_serio->name, sizeof(mlc_serio->name)-1, "HIL_SERIO%d", i);
 		snprintf(mlc_serio->name, sizeof(mlc_serio->name)-1, "HIL_SERIO%d", i);
 		snprintf(mlc_serio->phys, sizeof(mlc_serio->phys)-1, "HIL%d", i);
 		snprintf(mlc_serio->phys, sizeof(mlc_serio->phys)-1, "HIL%d", i);
 		mlc_serio->id			= hil_mlc_serio_id;
 		mlc_serio->id			= hil_mlc_serio_id;

+ 9 - 9
drivers/input/serio/hp_sdc_mlc.c

@@ -305,6 +305,7 @@ static void hp_sdc_mlc_out(hil_mlc *mlc)
 static int __init hp_sdc_mlc_init(void)
 static int __init hp_sdc_mlc_init(void)
 {
 {
 	hil_mlc *mlc = &hp_sdc_mlc;
 	hil_mlc *mlc = &hp_sdc_mlc;
+	int err;
 
 
 #ifdef __mc68000__
 #ifdef __mc68000__
 	if (!MACH_IS_HP300)
 	if (!MACH_IS_HP300)
@@ -323,22 +324,21 @@ static int __init hp_sdc_mlc_init(void)
 	mlc->out = &hp_sdc_mlc_out;
 	mlc->out = &hp_sdc_mlc_out;
 	mlc->priv = &hp_sdc_mlc_priv;
 	mlc->priv = &hp_sdc_mlc_priv;
 
 
-	if (hil_mlc_register(mlc)) {
+	err = hil_mlc_register(mlc);
+	if (err) {
 		printk(KERN_WARNING PREFIX "Failed to register MLC structure with hil_mlc\n");
 		printk(KERN_WARNING PREFIX "Failed to register MLC structure with hil_mlc\n");
-		goto err0;
+		return err;
 	}
 	}
 
 
 	if (hp_sdc_request_hil_irq(&hp_sdc_mlc_isr)) {
 	if (hp_sdc_request_hil_irq(&hp_sdc_mlc_isr)) {
 		printk(KERN_WARNING PREFIX "Request for raw HIL ISR hook denied\n");
 		printk(KERN_WARNING PREFIX "Request for raw HIL ISR hook denied\n");
-		goto err1;
+		if (hil_mlc_unregister(mlc))
+			printk(KERN_ERR PREFIX "Failed to unregister MLC structure with hil_mlc.\n"
+				"This is bad.  Could cause an oops.\n");
+		return -EBUSY;
 	}
 	}
+
 	return 0;
 	return 0;
- err1:
-	if (hil_mlc_unregister(mlc))
-		printk(KERN_ERR PREFIX "Failed to unregister MLC structure with hil_mlc.\n"
-			"This is bad.  Could cause an oops.\n");
- err0:
-	return -EBUSY;
 }
 }
 
 
 static void __exit hp_sdc_mlc_exit(void)
 static void __exit hp_sdc_mlc_exit(void)