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]

Message-ID: <20131220104519.GW5443@mwanda>
Date:	Fri, 20 Dec 2013 13:45:19 +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 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@...il.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
--
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/