[patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user()

[patch 1/2] Staging: intel_sst: return -EFAULT if copy_to_user()

[Dan Carpenter]

copy_to_user() returns the number of bytes remaining to be copied but we
want to return an error code.  And that error code is -EFAULT not -EIO.

Signed-off-by: Dan Carpenter <error27@gmail.com>

diff --git a/drivers/staging/intel_sst/intel_sst_app_interface.c b/drivers/staging/intel_sst/intel_sst_app_interface.c
index baf0ddc..5ead4d1 100644
--- a/drivers/staging/intel_sst/intel_sst_app_interface.c
+++ b/drivers/staging/intel_sst/intel_sst_app_interface.c
@@ -865,6 +865,8 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd, unsigned long arg)
 				str_info->src = SST_DRV;
 				retval = copy_to_user(&str_param->stream_id,
 						&retval, sizeof(__u32));
+				if (retval)
+					retval = -EFAULT;
 			} else {
 				if (retval = -SST_ERR_INVALID_PARAMS)
 					retval = -EINVAL;
@@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd, unsigned long arg)
 		retval = copy_to_user((struct snd_sst_vol *)arg,
 						&get_vol, sizeof(get_vol));
 		if (retval) {
-			retval = -EIO;
+			retval = -EFAULT;
 			break;
 		}
 		/*sst_print_get_vol_info(str_id, &get_vol);*/

RE: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Koul, Vinod]

 
> copy_to_user() returns the number of bytes remaining to be copied but we
> want to return an error code.  And that error code is -EFAULT not -EIO.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/staging/intel_sst/intel_sst_app_interface.c
> b/drivers/staging/intel_sst/intel_sst_app_interface.c
> index baf0ddc..5ead4d1 100644
> --- a/drivers/staging/intel_sst/intel_sst_app_interface.c
> +++ b/drivers/staging/intel_sst/intel_sst_app_interface.c
> @@ -865,6 +865,8 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> unsigned long arg)
>  				str_info->src = SST_DRV;
>  				retval = copy_to_user(&str_param->stream_id,
>  						&retval, sizeof(__u32));
> +				if (retval)
> +					retval = -EFAULT;
>  			} else {
>  				if (retval = -SST_ERR_INVALID_PARAMS)
>  					retval = -EINVAL;
> @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> unsigned long arg)
>  		retval = copy_to_user((struct snd_sst_vol *)arg,
>  						&get_vol, sizeof(get_vol));
>  		if (retval) {
> -			retval = -EIO;
> +			retval = -EFAULT;
>  			break;
>  		}
>  		/*sst_print_get_vol_info(str_id, &get_vol);*/
Since retval is not used and value assigned is supposed to be -EFAULT,
how about this?
	if(copy_to_user())
			return -EIO;

~Vinod

Re: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Dan Carpenter]

On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int cmd,
> > unsigned long arg)
> >  		retval = copy_to_user((struct snd_sst_vol *)arg,
> >  						&get_vol, sizeof(get_vol));
> >  		if (retval) {
> > -			retval = -EIO;
> > +			retval = -EFAULT;
> >  			break;
> >  		}
> >  		/*sst_print_get_vol_info(str_id, &get_vol);*/
> Since retval is not used and value assigned is supposed to be -EFAULT,
> how about this?
> 	if(copy_to_user())
> 			return -EIO;
> 

No. No.  retval is used.  We can't return directly because we need to
call unlock_kernel().

regards,
dan carpenter

RE: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Koul, Vinod]

> > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr, unsigned int
> cmd,
> > > unsigned long arg)
> > >  		retval = copy_to_user((struct snd_sst_vol *)arg,
> > >  						&get_vol, sizeof(get_vol));
> > >  		if (retval) {
> > > -			retval = -EIO;
> > > +			retval = -EFAULT;
> > >  			break;
> > >  		}
> > >  		/*sst_print_get_vol_info(str_id, &get_vol);*/
> > Since retval is not used and value assigned is supposed to be -EFAULT,
> > how about this?
> > 	if(copy_to_user())
> > 			return -EIO;
> >
> 
> No. No.  retval is used.  We can't return directly because we need to
> call unlock_kernel().
Kernel locking is already gone :) [1].

The point here was we don't need to store return value from this function
Anyway you could have done it this way
	if(copy_to_user()) {
		retval = -EIO;
		break;
	}


~Vinod

[1]: http://mailman.alsa-project.org/pipermail/alsa-devel/2010-October/032690.html

Re: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Alan Cox]

On Fri, 15 Oct 2010 05:44:07 +0200
Dan Carpenter <error27@gmail.com> wrote:

> copy_to_user() returns the number of bytes remaining to be copied but
> we want to return an error code.  And that error code is -EFAULT not
> -EIO.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>

Acked-by: Alan Cox <alan@linux.intel.com>

Re: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Alan Cox]

On Fri, 15 Oct 2010 07:29:01 +0200
Dan Carpenter <error27@gmail.com> wrote:

> On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr,
> > > unsigned int cmd, unsigned long arg)
> > >  		retval = copy_to_user((struct snd_sst_vol *)arg,
> > >  						&get_vol,
> > > sizeof(get_vol)); if (retval) {
> > > -			retval = -EIO;
> > > +			retval = -EFAULT;
> > >  			break;
> > >  		}
> > >  		/*sst_print_get_vol_info(str_id, &get_vol);*/
> > Since retval is not used and value assigned is supposed to be
> > -EFAULT, how about this?
> > 	if(copy_to_user())
> > 			return -EIO;
> > 
> 
> No. No.  retval is used.  We can't return directly because we need to
> call unlock_kernel().

Dan - your tree is a couple of patches stale at this point if it has
the lock_kernel in it - the lock_kernel has gone in the latest
submission.

Re: [patch 1/2] Staging: intel_sst: return -EFAULT if

[Dan Carpenter]

On Fri, Oct 15, 2010 at 01:59:48PM +0100, Alan Cox wrote:
> On Fri, 15 Oct 2010 07:29:01 +0200
> Dan Carpenter <error27@gmail.com> wrote:
> 
> > On Fri, Oct 15, 2010 at 09:53:57AM +0530, Koul, Vinod wrote:
> > > > @@ -938,7 +940,7 @@ long intel_sst_ioctl(struct file *file_ptr,
> > > > unsigned int cmd, unsigned long arg)
> > > >  		retval = copy_to_user((struct snd_sst_vol *)arg,
> > > >  						&get_vol,
> > > > sizeof(get_vol)); if (retval) {
> > > > -			retval = -EIO;
> > > > +			retval = -EFAULT;
> > > >  			break;
> > > >  		}
> > > >  		/*sst_print_get_vol_info(str_id, &get_vol);*/
> > > Since retval is not used and value assigned is supposed to be
> > > -EFAULT, how about this?
> > > 	if(copy_to_user())
> > > 			return -EIO;
> > > 
> > 
> > No. No.  retval is used.  We can't return directly because we need to
> > call unlock_kernel().
> 
> Dan - your tree is a couple of patches stale at this point if it has
> the lock_kernel in it - the lock_kernel has gone in the latest
> submission.

Yes.  That went in last night.  Vinod already told me.

I'll submit another patch which changes the file to the
	if (copy_to_user())
		retval = -EFAULT;
format.  It will apply on top of the patches I sent already.

There are some other bugs I noticed when I wrote that so it's worth it
to clean it up.

regards,
dan carpenter