[patch] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[patch] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[Dan Carpenter]

copy_to_user() returns the number of bytes remaining to be copied which
isn't the right thing to return here.  The comments say that these 
functions in dvb_ca_en50221.c should return the number of bytes copied or
an error return.  I've changed it to return -EFAULT.

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

diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
index ef259a0..aa7a298 100644
--- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
@@ -1318,8 +1318,10 @@ static ssize_t dvb_ca_en50221_io_write(struct file *file,
 
 		fragbuf[0] = connection_id;
 		fragbuf[1] = ((fragpos + fraglen) < count) ? 0x80 : 0x00;
-		if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
+		if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0) {
+			status = -EFAULT;
 			goto exit;
+		}
 
 		timeout = jiffies + HZ / 2;
 		written = 0;
@@ -1494,8 +1496,10 @@ static ssize_t dvb_ca_en50221_io_read(struct file *file, char __user * buf,
 
 	hdr[0] = slot;
 	hdr[1] = connection_id;
-	if ((status = copy_to_user(buf, hdr, 2)) != 0)
+	if ((status = copy_to_user(buf, hdr, 2)) != 0) {
+		status = -EFAULT;
 		goto exit;
+	}
 	status = pktlen;
 
 exit:

Re: [patch] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[walter harms]

Dan Carpenter schrieb:
> copy_to_user() returns the number of bytes remaining to be copied which
> isn't the right thing to return here.  The comments say that these 
> functions in dvb_ca_en50221.c should return the number of bytes copied or
> an error return.  I've changed it to return -EFAULT.
> 
> Signed-off-by: Dan Carpenter <error27@gmail.com>
> 
> diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
> index ef259a0..aa7a298 100644
> --- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
> +++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
> @@ -1318,8 +1318,10 @@ static ssize_t dvb_ca_en50221_io_write(struct file *file,
>  
>  		fragbuf[0] = connection_id;
>  		fragbuf[1] = ((fragpos + fraglen) < count) ? 0x80 : 0x00;
> -		if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
> +		if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0) {
> +			status = -EFAULT;
>  			goto exit;
> +		}
>  
>  		timeout = jiffies + HZ / 2;
>  		written = 0;
> @@ -1494,8 +1496,10 @@ static ssize_t dvb_ca_en50221_io_read(struct file *file, char __user * buf,
>  
>  	hdr[0] = slot;
>  	hdr[1] = connection_id;
> -	if ((status = copy_to_user(buf, hdr, 2)) != 0)
> +	if ((status = copy_to_user(buf, hdr, 2)) != 0) {
> +		status = -EFAULT;
>  		goto exit;
> +	}
>  	status = pktlen;
>  
>  exit:
> --


Doint to many things at once is bad. IMHO it is more readable to do so:

+status = copy_to_user(buf, hdr, 2);
+if ( status  != 0) {

Maybe the maintainer has different ideas but especialy lines like will gain.

-if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
+status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen):
+if ( status  != 0) {

just my 2 cents,
 wh

Re: [patch] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[Dan Carpenter]

On Fri, Jun 04, 2010 at 02:26:05PM +0200, walter harms wrote:
> 
> Doint to many things at once is bad. IMHO it is more readable to do so:
> 
> +status = copy_to_user(buf, hdr, 2);
> +if ( status  != 0) {
> 
> Maybe the maintainer has different ideas but especialy lines like will gain.
> 
> -if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
> +status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen):
> +if ( status  != 0) {
> 
> just my 2 cents,

You're right of course as always and checkpatch warns about these as
well.

I figured if it was in the original code, it was probably OK to leave it.
But I now recognize this as pure laziness on my part and I appologize.  
Twenty lashes for me and all that.  Fixed patch coming up.  ;)

regards,
dan carpenter

[patch v2] V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user errors

[Dan Carpenter]

copy_to_user() returns the number of bytes remaining to be copied which
isn't the right thing to return here.  The comments say that these 
functions in dvb_ca_en50221.c should return the number of bytes copied or
an error return.  I've changed it to return -EFAULT.

Signed-off-by: Dan Carpenter <error27@gmail.com>
---
V2: Some style fixes suggested by Walter Harms.

diff --git a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
index ef259a0..cb97e6b 100644
--- a/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
+++ b/drivers/media/dvb/dvb-core/dvb_ca_en50221.c
@@ -1318,8 +1318,11 @@ static ssize_t dvb_ca_en50221_io_write(struct file *file,
 
 		fragbuf[0] = connection_id;
 		fragbuf[1] = ((fragpos + fraglen) < count) ? 0x80 : 0x00;
-		if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0)
+		status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen);
+		if (status) {
+			status = -EFAULT;
 			goto exit;
+		}
 
 		timeout = jiffies + HZ / 2;
 		written = 0;
@@ -1494,8 +1497,11 @@ static ssize_t dvb_ca_en50221_io_read(struct file *file, char __user * buf,
 
 	hdr[0] = slot;
 	hdr[1] = connection_id;
-	if ((status = copy_to_user(buf, hdr, 2)) != 0)
+	status = copy_to_user(buf, hdr, 2);
+	if (status) {
+		status = -EFAULT;
 		goto exit;
+	}
 	status = pktlen;
 
 exit: