linux-kernel - [PATCH RFC] serio HIL MLC: don't deref null, don't leak and return proper error

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <alpine.LNX.2.00.1011072024100.26247@swampdragon.chaosbits.net>
Date:	Sun, 7 Nov 2010 20:36:33 +0100 (CET)
From:	Jesper Juhl <jj@...osbits.net>
To:	linux-input@...r.kernel.org
cc:	linux-kernel@...r.kernel.org,
	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Tejun Heo <tj@...nel.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	André Goddard Rosa <andre.goddard@...il.com>,
	Jiri Kosina <jkosina@...e.cz>
Subject: [PATCH RFC] serio HIL MLC: don't deref null, don't leak and return
 proper error

Hi,

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() buf 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 propagate it on 
failure and I also restructured the code to remove some labels and goto's 
to make it, IMHO nicer to read.

I have no hardware to test, so please review carefully and let me know if 
I've done something completely stupid. Please don't merge this RFC patch 
unless at least one or more people who know this code and can actually 
test it have ACK'ed it.

Ohh and please do CC me on replies.


Signed-off-by: Jesper Juhl <jj@...osbits.net>
---
 hil_mlc.c    |    5 +++++
 hp_sdc_mlc.c |   18 +++++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/input/serio/hil_mlc.c b/drivers/input/serio/hil_mlc.c
index e5624d8..bfd3865 100644
--- a/drivers/input/serio/hil_mlc.c
+++ b/drivers/input/serio/hil_mlc.c
@@ -932,6 +932,11 @@ int hil_mlc_register(hil_mlc *mlc)
 		hil_mlc_copy_di_scratch(mlc, i);
 		mlc_serio = kzalloc(sizeof(*mlc_serio), GFP_KERNEL);
 		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->phys, sizeof(mlc_serio->phys)-1, "HIL%d", i);
 		mlc_serio->id			= hil_mlc_serio_id;
diff --git a/drivers/input/serio/hp_sdc_mlc.c b/drivers/input/serio/hp_sdc_mlc.c
index 7d2b820..d50f067 100644
--- a/drivers/input/serio/hp_sdc_mlc.c
+++ b/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)
 {
 	hil_mlc *mlc = &hp_sdc_mlc;
+	int err;
 
 #ifdef __mc68000__
 	if (!MACH_IS_HP300)
@@ -323,22 +324,21 @@ static int __init hp_sdc_mlc_init(void)
 	mlc->out = &hp_sdc_mlc_out;
 	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");
-		goto err0;
+		return err;
 	}
 
 	if (hp_sdc_request_hil_irq(&hp_sdc_mlc_isr)) {
 		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;
- 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)



-- 
Jesper Juhl <jj@...osbits.net>             http://www.chaosbits.net/
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please.

--
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/