[PATCH v3] Staging: bcm: Fix potential buffer overflow and style cleanups

[Stephen Hemminger]

On Mon, 24 Jan 2011 13:45:02 +0100
Javier Martinez Canillas <martinez.javier at gmail.com> wrote:

> bcm driver copies from userpace with the following statement:
> 
> copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, IoBuffer.InputLength);
> 
> Compiling gives the following warning:
> 
> at drivers/staging/bcm/Bcmchar.c:2030:
> linux-next/arch/x86/include/asm/uaccess_32.h:212: warning: call to
> copy_from_user_overflow declared with attribute warning: copy_from_user()
> buffer size is not provably correct.
> 
> RxCntrlMsgBitMask is of type unsigned long so we have to check that
> IoBuffer.InputLength is equal to sizeof(unsigned long).
> 
> Signed-off-by: Javier Martinez Canillas <martinez.javier at gmail.com>
> ---
> 
>  V2: Check the user provided length recommended by Stephen Hemminger
>      Check for copy_from_user() and return -EFAULT; do not use ULONG and 
>      get rid of unnecessary log information recommended by Dan Carpenter.
>  V3: Check that user provided length is equal to sizeof(unsigned long) 
>      recommended by Dan Carpenter. Also, get rid of the innecesary scope 
>      brackets in the switch case.
> 
>  drivers/staging/bcm/Bcmchar.c |   46 +++++++++++++++++++++++-----------------
>  1 files changed, 26 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/staging/bcm/Bcmchar.c b/drivers/staging/bcm/Bcmchar.c
> index 31674ea..efea42e 100644
> --- a/drivers/staging/bcm/Bcmchar.c
> +++ b/drivers/staging/bcm/Bcmchar.c
> @@ -163,6 +163,7 @@ static long bcm_char_ioctl(struct file *filp, UINT cmd, ULONG arg)
>  	INT  			Status = STATUS_FAILURE;
>  	int timeout = 0;
>  	IOCTL_BUFFER 	IoBuffer;
> +	unsigned long RxCntrlMsgBitMask;
>  
>  	BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL, "Parameters Passed to control IOCTL cmd=0x%X arg=0x%lX", cmd, arg);
>  
> @@ -2015,28 +2016,33 @@ static long bcm_char_ioctl(struct file *filp, UINT cmd, ULONG arg)
>  				break ;
>  			 }
>  
> -		case IOCTL_BCM_CNTRLMSG_MASK:
> -			 {
> -				ULONG RxCntrlMsgBitMask = 0 ;
> +	case IOCTL_BCM_CNTRLMSG_MASK:
> +		RxCntrlMsgBitMask = 0;
> +		/* Copy Ioctl Buffer structure */
> +		Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
> +		if (Status) {
> +			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> +					"copy of Ioctl buffer is failed from user space");
> +			Status = -EFAULT;
> +			break;
> +		}
>  
> -				/* Copy Ioctl Buffer structure */
> -				Status = copy_from_user(&IoBuffer, argp, sizeof(IOCTL_BUFFER));
> -				if(Status)
> -				{
> -					BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of Ioctl buffer is failed from user space");
> -					break;
> -				}
> +		if (IoBuffer.InputLength != sizeof(unsigned long)) {
> +			Status = -EINVAL;
> +			break;
> +		}
>  
> -				Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer, IoBuffer.InputLength);
> -				if(Status)
> -				{
> -					BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"copy of control bit mask failed from user space");
> -					break;
> -				}
> -				BCM_DEBUG_PRINT(Adapter,DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,"\n Got user defined cntrl msg bit mask :%lx", RxCntrlMsgBitMask);
> -				pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask ;
> -			 }
> -			 break;
> +		Status = copy_from_user(&RxCntrlMsgBitMask, IoBuffer.InputBuffer,
> +					IoBuffer.InputLength);
> +		if (Status) {
> +			BCM_DEBUG_PRINT(Adapter, DBG_TYPE_OTHERS, OSAL_DBG, DBG_LVL_ALL,
> +					"copy of control bit mask failed from user space");
> +			Status = -EFAULT;
> +			break;
> +		}
> +
> +		pTarang->RxCntrlMsgBitMask = RxCntrlMsgBitMask;
> +		break;
>  			case IOCTL_BCM_GET_DEVICE_DRIVER_INFO:
>  			{
>  				DEVICE_DRIVER_INFO DevInfo;

1. Your indentation looks different than original code. 
2. Make RxCntrlMsgBitMask a block local variable like the rest of the
   code here.
3. Initialization is to zero is actually bad idea in this kind of code because
   automated tools can catch uninitialized variable usage and cause warning but
   by setting it to zero you defeat that.