linux-kernel - Re: [PATCH] drivers/staging/bcm: Integer overflow

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

Date:	Fri, 20 Dec 2013 11:16:33 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Wenliang Fan <fanwlexca@...il.com>
Cc:	gregkh@...uxfoundation.org, klmckinney1@...il.com,
	tulinizer@...il.com, devel@...verdev.osuosl.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drivers/staging/bcm: Integer overflow

On Fri, Dec 20, 2013 at 03:13:16PM +0800, Wenliang Fan wrote:
> The checking condition in 'validateFlash2xReadWrite()' is not sufficient.
> A large number invalid would cause an integer overflow and pass
> the condition, which could cause further integer overflows in
> 'Bcmchar.c:bcm_char_ioctl()'.
> 

This patch has a couple typos and it breaks the build.

On the other hand it's a real bug because uiNumOfBytes comes from the
user in bcm_char_ioctl().  This function doesn't seem to be restricted
to privaleged users, which is terrifying.

I think it would be cleaner to fix it in the caller instead of here.

Please look over this totally untested patch and send something like
that, if that's ok.

diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c
index 62415342ee28..2ea885e36077 100644
--- a/drivers/staging/bcm/Bcmchar.c
+++ b/drivers/staging/bcm/Bcmchar.c
@@ -1456,8 +1456,7 @@ cntrlEnd:
 	case IOCTL_BCM_FLASH2X_SECTION_READ: {
 		struct bcm_flash2x_readwrite sFlash2xRead = {0};
 		PUCHAR pReadBuff = NULL;
-		UINT NOB = 0;
-		UINT BuffSize = 0;
+		UINT NOB;
 		UINT ReadBytes = 0;
 		UINT ReadOffset = 0;
 		void __user *OutPutBuff;
@@ -1480,19 +1479,17 @@ cntrlEnd:
 		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, "\nsFlash2xRead.numOfBytes :%x", sFlash2xRead.numOfBytes);
 		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, "\nsFlash2xRead.bVerify :%x\n", sFlash2xRead.bVerify);
 
+		if (sFlash2xRead.numOfBytes > Adapter->uiSectorSize)
+			sFlash2xRead.numOfBytes = Adapter->uiSectorSize;
+		NOB = sFlash2xRead.numOfBytes;
+
 		/* This was internal to driver for raw read. now it has ben exposed to user space app. */
 		if (validateFlash2xReadWrite(Adapter, &sFlash2xRead) == false)
 			return STATUS_FAILURE;
 
-		NOB = sFlash2xRead.numOfBytes;
-		if (NOB > Adapter->uiSectorSize)
-			BuffSize = Adapter->uiSectorSize;
-		else
-			BuffSize = NOB;
-
 		ReadOffset = sFlash2xRead.offset;
 		OutPutBuff = IoBuffer.OutputBuffer;
-		pReadBuff = (PCHAR)kzalloc(BuffSize , GFP_KERNEL);
+		pReadBuff = (PCHAR)kzalloc(sFlash2xRead.numOfBytes, GFP_KERNEL);
 
 		if (pReadBuff == NULL) {
 			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "Memory allocation failed for Flash 2.x Read Structure");
@@ -1548,8 +1545,7 @@ cntrlEnd:
 		struct bcm_flash2x_readwrite sFlash2xWrite = {0};
 		PUCHAR pWriteBuff;
 		void __user *InputAddr;
-		UINT NOB = 0;
-		UINT BuffSize = 0;
+		UINT NOB;
 		UINT WriteOffset = 0;
 		UINT WriteBytes = 0;
 
@@ -1580,19 +1576,17 @@ cntrlEnd:
 			return -EINVAL;
 		}
 
+		if (sFlash2xWrite.numOfBytes > Adapter->uiSectorSize)
+			sFlash2xWrite.numOfBytes = Adapter->uiSectorSize;
+		NOB = sFlash2xWrite.numOfBytes;
+
 		if (validateFlash2xReadWrite(Adapter, &sFlash2xWrite) == false)
 			return STATUS_FAILURE;
 
 		InputAddr = sFlash2xWrite.pDataBuff;
 		WriteOffset = sFlash2xWrite.offset;
-		NOB = sFlash2xWrite.numOfBytes;
-
-		if (NOB > Adapter->uiSectorSize)
-			BuffSize = Adapter->uiSectorSize;
-		else
-			BuffSize = NOB;
 
-		pWriteBuff = kmalloc(BuffSize, GFP_KERNEL);
+		pWriteBuff = kmalloc(sFlash2xWrite.numOfBytes, GFP_KERNEL);
 
 		if (pWriteBuff == NULL)
 			return -ENOMEM;
--
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/