[PATCH] Staging: bcm: Fix information leak in ioctl, IOCTL_BCM_REGISTER_READ_PRIVATE

Dan Carpenter dan.carpenter at oracle.com
Fri Oct 28 08:27:17 UTC 2011


The patch has more than one place where we accidentally return
positive numbers on success.  I'm going to point one place, can you
please find and fix the others?

On Thu, Oct 27, 2011 at 07:40:53PM -0400, Kevin McKinney wrote:
> --- a/drivers/staging/bcm/nvm.c
> +++ b/drivers/staging/bcm/nvm.c
> @@ -443,7 +443,7 @@ static INT BeceemFlashBulkRead(
>  {
>  	UINT uiIndex = 0;
>  	UINT uiBytesToRead = uiNumBytes;
> -	INT Status = 0;
> +	int Status;
>  	UINT uiPartOffset = 0;
>  
>  	if(Adapter->device_removed )
> @@ -469,8 +469,8 @@ static INT BeceemFlashBulkRead(
>  		uiBytesToRead = MAX_RW_SIZE - (uiOffset%MAX_RW_SIZE);
>  		uiBytesToRead = MIN(uiNumBytes,uiBytesToRead);
>  
> -		if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
> -		{
> +		Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead); 
> +		if (Status < 0) {
>  			Status = -1;
>  			Adapter->SelectedChip = RESET_CHIP_SELECT;
>  			return Status;
> @@ -488,8 +488,8 @@ static INT BeceemFlashBulkRead(
>  
>  		uiBytesToRead = MIN(uiNumBytes,MAX_RW_SIZE);
>  
> -		if(rdm(Adapter,uiPartOffset, (PCHAR)pBuffer+uiIndex,uiBytesToRead))
> -		{
> +		Status = rdm(Adapter, uiPartOffset, (PCHAR)pBuffer + uiIndex, uiBytesToRead);
> +		if (Status < 0) {
>  			Status = -1;
>  			break;
>  		}

Status will be positive at the end of this function.

Btw, you could probably get rid of the "Status = -1;" assignment from
the original code.  -1 is the wrong error code here and your new code
is preserving error codes.

InterfaceAdapterInit() also has a problem caused by a positive
"retval".  There is at least one other place I have not mentioned
because I am a jerk.  There may be more than one because I have not
audited everything completely.

Part of the problem I think is that we have Status and retval in the
same function.  The names of those things mean exactly the same thing
and they are storing similar data.  Maybe the thing is to say that
rdm() always returns to a variable called count.

Example 1:
	count = rdm();
	if (count < 0)
		return count;

Example 2:
	count = rdm();
	if (count < 0) {
		retval = count;
		break;
	}

Example 3:
	The ioctl version is the only place where we care about
	positive values of count.

	count = rdm();
	if (count < 0) {
		retval = count;
	} else {
		if (copy_to_user(foo, bar, count))
			retval = -EFAULT;
	}

I think that would be less error prone.

regards,
dan carpenter




More information about the devel mailing list