Re: [PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

Re: [PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

[dan carpenter]

On Sunday 25 May 2003 06:28 pm, Jens Axboe wrote:
> On Sun, May 25 2003, Paulo Andre' wrote:
> > Hi Jens,
> >
> > Please find attached a trivial patch that checks both
> > copy_to_user() and copy_from_user() returns values in scsi_ioctl.c,
> > returning accordinly in case of a transfer error.
>
> See above, we've already done access_ok() on the buffer so the
> "unchecked" copy_to/from_user are done that way on purpose. I suppose it
> could be made more explicit with __copy_to/from_user().

access_ok() doesn't seem to mean copy_to_user will return 0.

438 unsigned long copy_to_user(void *to, const void *from, unsigned long n)
439 {
440         prefetch(from);
441         if (access_ok(VERIFY_WRITE, to, n))
442                 n = __copy_to_user(to, from, n);
443         return n;
444 }

I have a script that finds all the unchecked calls to copy_to_user() and
I am curious about what cases it does not need to be checked.

http://kbugs.org/cgi-bin/index.py?page=bug_list&&script=UncheckedReturn&skernel=2.5.69&sfile=&start_bug=0&

Thanks,
dan carpenter

[PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

[Paulo Andre']

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

Hi Jens,

Please find attached a trivial patch that checks both
copy_to_user() and copy_from_user() returns values in scsi_ioctl.c,
returning accordinly in case of a transfer error.

Please review.


		Paulo Andre'




[-- Attachment #2: patch-scsi_ioctl.c.diff --]
[-- Type: text/plain, Size: 1248 bytes --]

--- scsi_ioctl.c.orig	2003-05-25 16:42:22.000000000 +0100
+++ scsi_ioctl.c	2003-05-25 16:59:44.000000000 +0100
@@ -213,7 +213,8 @@
 
 			nr_sectors = bytes >> 9;
 			if (writing)
-				copy_from_user(buffer,hdr.dxferp,hdr.dxfer_len);
+				if (copy_from_user(buffer,hdr.dxferp,hdr.dxfer_len))
+					goto efault;
 			else
 				memset(buffer, 0, hdr.dxfer_len);
 		}
@@ -225,7 +226,8 @@
 	 * fill in request structure
 	 */
 	rq->cmd_len = hdr.cmd_len;
-	copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len);
+	if (copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len))
+		goto efault;
 	if (sizeof(rq->cmd) != hdr.cmd_len)
 		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
 
@@ -286,17 +288,23 @@
 
 	blk_put_request(rq);
 
-	copy_to_user(uptr, &hdr, sizeof(*uptr));
+	if (copy_to_user(uptr, &hdr, sizeof(*uptr)))
+		goto efault;
 
 	if (buffer) {
 		if (reading)
-			copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len);
+			if (copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len))
+				goto efault;
 
 		kfree(buffer);
 	}
 	/* may not have succeeded, but output values written to control
 	 * structure (struct sg_io_hdr).  */
 	return 0;
+efault:
+	if (buffer)
+		kfree(buffer);
+	return -EFAULT;
 }
 
 #define FORMAT_UNIT_TIMEOUT		(2 * 60 * 60 * HZ)

Re: [PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

[Jens Axboe]

On Sun, May 25 2003, Paulo Andre' wrote:
> Hi Jens,
> 
> Please find attached a trivial patch that checks both
> copy_to_user() and copy_from_user() returns values in scsi_ioctl.c,
> returning accordinly in case of a transfer error.

See above, we've already done access_ok() on the buffer so the
"unchecked" copy_to/from_user are done that way on purpose. I suppose it
could be made more explicit with __copy_to/from_user().

-- 
Jens Axboe

Re: [PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

[Oliver Neukum]

Am Sonntag, 25. Mai 2003 18:28 schrieb Jens Axboe:
> On Sun, May 25 2003, Paulo Andre' wrote:
> > Hi Jens,
> >
> > Please find attached a trivial patch that checks both
> > copy_to_user() and copy_from_user() returns values in scsi_ioctl.c,
> > returning accordinly in case of a transfer error.
>
> See above, we've already done access_ok() on the buffer so the
> "unchecked" copy_to/from_user are done that way on purpose. I suppose it
> could be made more explicit with __copy_to/from_user().

Doesn't this race with munmap on SMP?

	Regards
		Oliver

Re: [PATCH] Check copy_*_user return value in drivers/block/scsi_ioctl.c

[Paulo Andre']

[-- Attachment #1: Type: text/plain, Size: 348 bytes --]

On Sun, 25 May 2003 18:28:44 +0200
Jens Axboe <axboe@suse.de> wrote:

> 
> See above, we've already done access_ok() on the buffer so the
> "unchecked" copy_to/from_user are done that way on purpose. I suppose
> it could be made more explicit with __copy_to/from_user().

Ok, makes sense indeed. Please consider the following patch then.


		Paulo

[-- Attachment #2: patch-scsi_ioctl.c.diff --]
[-- Type: text/plain, Size: 941 bytes --]

--- scsi_ioctl.c.orig	2003-05-25 16:42:22.000000000 +0100
+++ scsi_ioctl.c	2003-05-25 17:54:21.000000000 +0100
@@ -213,7 +213,7 @@
 
 			nr_sectors = bytes >> 9;
 			if (writing)
-				copy_from_user(buffer,hdr.dxferp,hdr.dxfer_len);
+				__copy_from_user(buffer,hdr.dxferp,hdr.dxfer_len);
 			else
 				memset(buffer, 0, hdr.dxfer_len);
 		}
@@ -225,7 +225,7 @@
 	 * fill in request structure
 	 */
 	rq->cmd_len = hdr.cmd_len;
-	copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len);
+	__copy_from_user(rq->cmd, hdr.cmdp, hdr.cmd_len);
 	if (sizeof(rq->cmd) != hdr.cmd_len)
 		memset(rq->cmd + hdr.cmd_len, 0, sizeof(rq->cmd) - hdr.cmd_len);
 
@@ -286,11 +286,11 @@
 
 	blk_put_request(rq);
 
-	copy_to_user(uptr, &hdr, sizeof(*uptr));
+	__copy_to_user(uptr, &hdr, sizeof(*uptr));
 
 	if (buffer) {
 		if (reading)
-			copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len);
+			__copy_to_user(hdr.dxferp, buffer, hdr.dxfer_len);
 
 		kfree(buffer);
 	}