V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user
Subsystems (2)
PATCH 1/1 V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user
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.
drivers/media/dvb/dvb-core/dvb_ca_en50221.c +6 -2
@@ -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:
Quoted 26 lines · 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 indvb_ca_en50221.cshould return the number of bytes copied or an error return. I've changed it to return-EFAULT.Signed-off-by Dan Carpenter <[email protected]>drivers/media/dvb/dvb-core/dvb_ca_en50221.c +3
-1@@ -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;
drivers/media/dvb/dvb-core/dvb_ca_en50221.c static ssize_t dvb_ca_en50221_io_read(struct file *file, char __user * buf,
Quoted 10 lines
drivers/media/dvb/dvb-core/dvb_ca_en50221.c +3
-2hdr[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
Quoted 12 lines · walter harms
Doint to many things at once is bad. IMHO it is more readable to do so:
(quoted diff) +2
-0+status = copy_to_user(buf, hdr, 2); +if ( status != 0) {Maybe the maintainer has different ideas but especialy lines like will gain.
(quoted diff) +2
-1-if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0) +status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen): +if ( status != 0) {
Quoted 1 line
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 1/1 V4L/DVB: dvb_ca_en50221: return -EFAULT on copy_to_user
In reply to walter harmscopy_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.
---
V2: Some style fixes suggested by Walter Harms.
drivers/media/dvb/dvb-core/dvb_ca_en50221.c +8 -2
@@ -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:
Quoted 17 lines · Dan Carpenter
Quoted 11 lines · walter harms
Doint to many things at once is bad. IMHO it is more readable to do so:
(quoted diff) +2
-0+status = copy_to_user(buf, hdr, 2); +if ( status != 0) {Maybe the maintainer has different ideas but especialy lines like will gain.
(quoted diff) +2
-1-if ((status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen)) != 0) +status = copy_from_user(fragbuf + 2, buf + fragpos, fraglen): +if ( status != 0) {Quoted 1 line
just my 2 cents,
You're right of course as always and checkpatch warns about these as well.
Quoted 6 lines
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
I hope my guidance will ease your way to enlightenment. ;)
re,
wh