Re: [PATCH 2/3] cciss: bug fix for BIG_PASS_THRU

[mikem <mikem@beardog.cca.cpqcorp.net>]

On Fri, Nov 18, 2005 at 10:01:24PM +0100, Jens Axboe wrote:
> On Fri, Nov 18 2005, mikem wrote:
> > Patch 2 of 3
> > 
> > Applications using CCISS_BIG_PASSTHRU complained that the data written
> > was zeros.  The code looked alright, but it seems that copy_from_user 
> > already does a memset on the buffer. Removing it from the pass-through
> > fixes the apps.
> 
> Hmm, I don't like this patch, since you never clear the buffer for reads
> now. If the controller for some reason doesn't overwrite this buffer,
> you could be leaking privileged data! Your bug is because you do:
> 
>         if (write && copy_from_user(...))
>                 fail
>         else
>                 clear
> 
> so you end up in the clear case for any case where copy_from_user()
> doesn't fail. I've fixed it up for you, this is what I committed:

Thanks, Jens.

> 
> diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
> index e239a6c..33f8341 100644
> --- a/drivers/block/cciss.c
> +++ b/drivers/block/cciss.c
> @@ -1017,10 +1017,11 @@ static int cciss_ioctl(struct inode *ino
>  				status = -ENOMEM;
>  				goto cleanup1;
>  			}
> -			if (ioc->Request.Type.Direction == XFER_WRITE &&
> -				copy_from_user(buff[sg_used], data_ptr, sz)) {
> +			if (ioc->Request.Type.Direction == XFER_WRITE) {
> +				if (copy_from_user(buff[sg_used], data_ptr, sz)) {
>  					status = -ENOMEM;
> -					goto cleanup1;			
> +					goto cleanup1;
> +				}
>  			} else {
>  				memset(buff[sg_used], 0, sz);
>  			}
> 
> -- 
> Jens Axboe
>