linux-kernel - [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty

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-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <1428415875-23797-1-git-send-email-giedrius.statkevicius@gmail.com>
Date:	Tue,  7 Apr 2015 17:11:15 +0300
From:	Giedrius Statkevičius 
	<giedrius.statkevicius@...il.com>
To:	lidza.louina@...il.com, markh@...pro.net
Cc:	gregkh@...uxfoundation.org, driverdev-devel@...uxdriverproject.org,
	devel@...verdev.osuosl.org, linux-kernel@...r.kernel.org,
	dan.carpenter@...cle.com, sudipm.mukherjee@...il.com,
	Giedrius Statkevičius 
	<giedrius.statkevicius@...il.com>
Subject: [PATCH v3] staging: dgnc: check if kzalloc fails in dgnc_tty_init()

If one of the allocations of memory for storing a channel information struct
fails then free all the successful allocations and return -ENOMEM that gets
propogated to the pci layer.  Also, remove a bogus skipping in the next part of
the initiation if a previous memory allocation failed because we won't execute
that if any of the allocations failed. Next, remove the misleading comment that
allocation could happen elsewhere. Finally, remove all (except in the ioctl
which can try to get information about a board that failed to probe) checks if
->channels[foo] is NULL or not because probe failing if we can't allocate enough
memory means that this scenario isn't possible.

Signed-off-by: Giedrius Statkevičius <giedrius.statkevicius@...il.com>
---
v3: Remove the wrong comment at dgnc_tty_init() that says the allocation could
happen somewhere else before this and remove the check if (!brd->channels[i]).
Also, remove all checks where ->channels[i] is checked if it's NULL or not
because if probe fails then that means that this scenario isn't possible (except
in the ioctl).

v2: Only returning -ENOMEM if an allocation failed isn't enough as it was
spotted by Sudip so create a new label that frees all successfully allocated
stuff and only then returns -ENOMEM. Also, remove a unnecessary check in the
next loop.

 drivers/staging/dgnc/dgnc_cls.c |  4 +---
 drivers/staging/dgnc/dgnc_neo.c |  2 +-
 drivers/staging/dgnc/dgnc_tty.c | 29 +++++++++++++----------------
 3 files changed, 15 insertions(+), 20 deletions(-)

diff --git a/drivers/staging/dgnc/dgnc_cls.c b/drivers/staging/dgnc/dgnc_cls.c
index e3564d2..a629a78 100644
--- a/drivers/staging/dgnc/dgnc_cls.c
+++ b/drivers/staging/dgnc/dgnc_cls.c
@@ -379,7 +379,7 @@ static inline void cls_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
@@ -714,8 +714,6 @@ static void cls_tasklet(unsigned long data)
 		/* Loop on each port */
 		for (i = 0; i < ports; i++) {
 			ch = bd->channels[i];
-			if (!ch)
-				continue;
 
 			/*
 			 * NOTE: Remember you CANNOT hold any channel
diff --git a/drivers/staging/dgnc/dgnc_neo.c b/drivers/staging/dgnc/dgnc_neo.c
index f5a4d36..1e583c2 100644
--- a/drivers/staging/dgnc/dgnc_neo.c
+++ b/drivers/staging/dgnc/dgnc_neo.c
@@ -395,7 +395,7 @@ static inline void neo_parse_isr(struct dgnc_board *brd, uint port)
 		return;
 
 	ch = brd->channels[port];
-	if (!ch || ch->magic != DGNC_CHANNEL_MAGIC)
+	if (ch->magic != DGNC_CHANNEL_MAGIC)
 		return;
 
 	/* Here we try to figure out what caused the interrupt to happen */
diff --git a/drivers/staging/dgnc/dgnc_tty.c b/drivers/staging/dgnc/dgnc_tty.c
index ce4187f..d1a2c0f 100644
--- a/drivers/staging/dgnc/dgnc_tty.c
+++ b/drivers/staging/dgnc/dgnc_tty.c
@@ -304,19 +304,15 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	brd->nasync = brd->maxports;
 
-	/*
-	 * Allocate channel memory that might not have been allocated
-	 * when the driver was first loaded.
-	 */
 	for (i = 0; i < brd->nasync; i++) {
-		if (!brd->channels[i]) {
-
-			/*
-			 * Okay to malloc with GFP_KERNEL, we are not at
-			 * interrupt context, and there are no locks held.
-			 */
-			brd->channels[i] = kzalloc(sizeof(*brd->channels[i]), GFP_KERNEL);
-		}
+		/*
+		 * Okay to malloc with GFP_KERNEL, we are not at
+		 * interrupt context, and there are no locks held.
+		 */
+		brd->channels[i] = kzalloc(sizeof(*brd->channels[i]),
+					   GFP_KERNEL);
+		if (!brd->channels[i])
+			goto err_free_channels;
 	}
 
 	ch = brd->channels[0];
@@ -324,10 +320,6 @@ int dgnc_tty_init(struct dgnc_board *brd)
 
 	/* Set up channel variables */
 	for (i = 0; i < brd->nasync; i++, ch = brd->channels[i]) {
-
-		if (!brd->channels[i])
-			continue;
-
 		spin_lock_init(&ch->ch_lock);
 
 		/* Store all our magic numbers */
@@ -375,6 +367,11 @@ int dgnc_tty_init(struct dgnc_board *brd)
 	}
 
 	return 0;
+
+err_free_channels:
+	for (i = i - 1; i >= 0; --i)
+		kfree(brd->channels[i]);
+	return -ENOMEM;
 }
 
 
-- 
2.3.5

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