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

[Dan Carpenter <dan.carpenter@oracle.com>]

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