[PATCH] drivers/staging/bcm: Integer overflow

[PATCH] drivers/staging/bcm: Integer overflow

[Wenliang Fan]

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()'.

Signed-off-by: Wenliang Fan <fanwlexca@gmail.com>
---
 drivers/staging/bcm/nvm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
index 9e5f955..0615609 100644
--- a/drivers/staging/bcm/nvm.c
+++ b/drivers/staging/bcm/nvm.c
@@ -3945,7 +3945,9 @@ int validateFlash2xReadWrite(struct bcm_mini_adapter *Adapter, struct bcm_flash2
 	BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, NVM_RW, DBG_LVL_ALL, "End offset :%x\n", uiSectEndOffset);
 
 	/* Checking the boundary condition */
-	if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset)
+	if (((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset)
+			&& (uiSectStartOffset + psFlash2xReadWrite->offset > uiSectStartOffset)
+			&& (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumBytes > uiNumBytes))
 		return TRUE;
 	else {
 		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "Invalid Request....");
-- 
1.8.5.rc1.28.g7061504

Re: [PATCH] drivers/staging/bcm: Integer overflow

[Dan Carpenter]

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;

Re: [PATCH] drivers/staging/bcm: Integer overflow

[Dan Carpenter]

On Fri, Dec 20, 2013 at 04:51:45PM +0800, Wenliang Fan wrote:
> Thanks for your advice.
> But the variable 'psFlash2xReadWrite->offset' in '
> *drivers/staging/bcm/nvm.c*:validateFlash2xReadWrite()' is also comes from
> user space, which would cause an integer overflow in the following line:
> 
> if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <=
> uiSectEndOffset)
> 
> in '*drivers/staging/bcm/**nvm.c*: validateFlash2xReadWrite()',
> and another integer overflow in the following line:
> 
> 
> ReadOffset = ReadOffset + ReadBytes; (or WriteOffset = WriteOffset +
> WriteBytes;)
> 
> in '*drivers/staging/bcm/**Bcmchar.c*: bcm_char_ioctl()'.
> 

Alright, fine.  But the new check is messy.  Do it like this:

	/* these are user controlled and can lead to integer overflows */
	if (psFlash2xReadWrite->offset > uiSectEndOffset)
		return false;
	if (uiNumOfBytes > uiSectEndOffset)
		return false;
	if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes > uiSectEndOffset)
		return false;

	return true;

That way each step is simpler to understand.  People are too fond of
compound conditions... *grumble*.

regards,
dan carpenter

[PATCH] drivers/staging/bcm: Integer overflow

[Wenliang Fan]

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()'.

Signed-off-by: Wenliang Fan <fanwlexca@gmail.com>
---
 drivers/staging/bcm/nvm.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
index 9e5f955..7f3dd4b 100644
--- a/drivers/staging/bcm/nvm.c
+++ b/drivers/staging/bcm/nvm.c
@@ -3944,6 +3944,11 @@ int validateFlash2xReadWrite(struct bcm_mini_adapter *Adapter, struct bcm_flash2
 
 	BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, NVM_RW, DBG_LVL_ALL, "End offset :%x\n", uiSectEndOffset);
 
+	/* psFlash2xReadWrite->offset and uiNumOfBytes are user controlled and can lead to integer overflows */
+	if (uiSectStartOffset + psFlash2xReadWrite->offset < uiSectStartOffset)
+		return false;
+	if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes < uiNumOfBytes)
+		return false;
 	/* Checking the boundary condition */
 	if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset)
 		return TRUE;
-- 
1.8.5.rc1.28.g7061504

Re: [PATCH] drivers/staging/bcm: Integer overflow

[Dan Carpenter]

On Fri, Dec 20, 2013 at 06:19:56PM +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()'.
> 
> Signed-off-by: Wenliang Fan <fanwlexca@gmail.com>
> ---
>  drivers/staging/bcm/nvm.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
> index 9e5f955..7f3dd4b 100644
> --- a/drivers/staging/bcm/nvm.c
> +++ b/drivers/staging/bcm/nvm.c
> @@ -3944,6 +3944,11 @@ int validateFlash2xReadWrite(struct bcm_mini_adapter *Adapter, struct bcm_flash2
>  
>  	BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, NVM_RW, DBG_LVL_ALL, "End offset :%x\n", uiSectEndOffset);
>  
> +	/* psFlash2xReadWrite->offset and uiNumOfBytes are user controlled and can lead to integer overflows */
> +	if (uiSectStartOffset + psFlash2xReadWrite->offset < uiSectStartOffset)
> +		return false;
> +	if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes < uiNumOfBytes)
> +		return false;

Please don't do this...  :( Just do it exactly like I explained before.

Using this style of overflow checking causes static checkers which look
for integer overflows to complain and it is too complicated.

Just do:

	if (uiSectStartOffset > uiSectEndOffset)
		return false;
	if (psFlash2xReadWrite->offset > uiSectEndOffset)
		return false;

>  	/* Checking the boundary condition */
>  	if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset)
>  		return TRUE;

Reverse this condition so it does:
	if (uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes > uiSectEndOffset)
		return false;

Then if everything is valid do:

	return true;

Compare how these two statements sound in English:

	If psFlash2xReadWrite->offset is larger than uiSectEndOffset
	return false.

Vs:
	If we uiSectStartOffset plus psFlash2xReadWrite->offset plus
	uiNumOfBytes is less than uiNumOfBytes then it means we
	overflowed.  Since we verified that uiSectStartOffset plus
	psFlash2xReadWrite->offset don't overflow that means
	uiNumOfBytes is too large so return false.

The first one is simple and the second one is complicated.  Don't do
complicated things for no reason.  Also the first one is fewer
operations for the CPU.

regards,
dan carpenter

[PATCH] drivers/staging/bcm: Integer overflow

[Wenliang Fan]

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()'.

Signed-off-by: Wenliang Fan <fanwlexca@gmail.com>
---
 drivers/staging/bcm/nvm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/staging/bcm/nvm.c b/drivers/staging/bcm/nvm.c
index 9e5f955..3165da8 100644
--- a/drivers/staging/bcm/nvm.c
+++ b/drivers/staging/bcm/nvm.c
@@ -3944,6 +3944,15 @@ int validateFlash2xReadWrite(struct bcm_mini_adapter *Adapter, struct bcm_flash2
 
 	BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, NVM_RW, DBG_LVL_ALL, "End offset :%x\n", uiSectEndOffset);
 
+	/* psFlash2xReadWrite->offset and uiNumOfBytes are user controlled and can lead to integer overflows */
+	if (psFlash2xReadWrite->offset > uiSectEndOffset) {
+		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "Invalid Request....");
+		return false;
+	}
+	if (uiNumOfBytes > uiSectEndOffset) {
+		BCM_DEBUG_PRINT(Adapter, DBG_TYPE_PRINTK, 0, 0, "Invalid Request....");
+		return false;
+	}
 	/* Checking the boundary condition */
 	if ((uiSectStartOffset + psFlash2xReadWrite->offset + uiNumOfBytes) <= uiSectEndOffset)
 		return TRUE;
-- 
1.8.5.rc1.28.g7061504

Re: [PATCH] drivers/staging/bcm: Integer overflow

[Dan Carpenter]

On Fri, Dec 20, 2013 at 07:07:38PM +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()'.
> 
> Signed-off-by: Wenliang Fan <fanwlexca@gmail.com>

Looks good.

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter