linux-kernel - [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section

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: <8738m2uukz.fsf@mina86.com>
Date:	Mon, 09 Dec 2013 12:34:04 +0100
From:	Michal Nazarewicz <mina86@...a86.com>
To:	Eugene Krasnikov <k.eugene.e@...il.com>
Cc:	Peter Stuge <peter@...ge.se>,
	"John W. Linville" <linville@...driver.com>,
	ath9k-devel <ath9k-devel@...ts.ath9k.org>,
	linux-wireless <linux-wireless@...r.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: [PATCH] net: wirelesse: wcn36xx: check allocation and trim critical section

Commit [3469adb3: fix potential NULL pointer dereference] introduced
a check of msg_ind allocation, but omitted allocation of msg_ind->msg.
Moreover, it introduced two if statements, which looked a bit clunky.

This commit moves allocation code outside of the critical section so
there's no need to dance around mutex_unlock, and adds the missing
allocation check.

---
 drivers/net/wireless/ath/wcn36xx/smd.c | 32 +++++++++++++++++++-------------
 1 file changed, 19 insertions(+), 13 deletions(-)

On Mon, Dec 09 2013, Eugene Krasnikov wrote:
> hal_ind_mutex suppose to protect msg_ind but with this patch
> allocation will be done outside the critical section.

msg_ind is a local variable which is not shared outside of the function
until it's added to a list.  I don't see how it matters if it's
allocation and initialisation is done under mutex.

In fact, there's another kmalloc call that should be checked and can be
done outside of the critical section, so disregard my previous patch,
here's a new one.

diff --git a/drivers/net/wireless/ath/wcn36xx/smd.c b/drivers/net/wireless/ath/wcn36xx/smd.c
index 823631c..713e3ce 100644
--- a/drivers/net/wireless/ath/wcn36xx/smd.c
+++ b/drivers/net/wireless/ath/wcn36xx/smd.c
@@ -2056,22 +2056,28 @@ static void wcn36xx_smd_rsp_process(struct wcn36xx *wcn, void *buf, size_t len)
 	case WCN36XX_HAL_OTA_TX_COMPL_IND:
 	case WCN36XX_HAL_MISSED_BEACON_IND:
 	case WCN36XX_HAL_DELETE_STA_CONTEXT_IND:
-		mutex_lock(&wcn->hal_ind_mutex);
 		msg_ind = kmalloc(sizeof(*msg_ind), GFP_KERNEL);
-		if (msg_ind) {
-			msg_ind->msg_len = len;
-			msg_ind->msg = kmalloc(len, GFP_KERNEL);
-			memcpy(msg_ind->msg, buf, len);
-			list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
-			queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
-			wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
+		if (!msg_ind)
+			goto nomem;
+		msg_ind->msg_len = len;
+		msg_ind->msg = kmalloc(len, GFP_KERNEL);
+		if (!msg_ind->msg) {
+			kfree(msg_ind);
+nomem:
+			/*
+			 * FIXME: Do something smarter then just
+			 * printing an error.
+			 */
+			wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
+				    msg_header->msg_type);
+			break;
 		}
+		memcpy(msg_ind->msg, buf, len);
+		mutex_lock(&wcn->hal_ind_mutex);
+		list_add_tail(&msg_ind->list, &wcn->hal_ind_queue);
+		queue_work(wcn->hal_ind_wq, &wcn->hal_ind_work);
 		mutex_unlock(&wcn->hal_ind_mutex);
-		if (msg_ind)
-			break;
-		/* FIXME: Do something smarter then just printing an error. */
-		wcn36xx_err("Run out of memory while handling SMD_EVENT (%d)\n",
-			    msg_header->msg_type);
+		wcn36xx_dbg(WCN36XX_DBG_HAL, "indication arrived\n");
 		break;
 	default:
 		wcn36xx_err("SMD_EVENT (%d) not supported\n",
-- 
1.8.4


Download attachment "signature.asc" of type "application/pgp-signature" (836 bytes)