[PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jesper Juhl]

Hi,

Here's a patch to ensure that the return value from __copy_to_user() gets 
checked in cdrom_read_cdda_old().
I assume that returning -EFAULT if the copy fails to copy all bytes is an 
appropriate action, but please correct me if I'm wrong.

Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>

diff -up linux-2.6.9-rc1-bk13-orig/drivers/cdrom/cdrom.c linux-2.6.9-rc1-bk13/drivers/cdrom/cdrom.c
--- linux-2.6.9-rc1-bk13-orig/drivers/cdrom/cdrom.c	2004-08-24 20:44:01.000000000 +0200
+++ linux-2.6.9-rc1-bk13/drivers/cdrom/cdrom.c	2004-09-06 23:41:20.000000000 +0200
@@ -1959,7 +1959,10 @@ static int cdrom_read_cdda_old(struct cd
 		ret = cdrom_read_block(cdi, &cgc, lba, nr, 1, CD_FRAMESIZE_RAW);
 		if (ret)
 			break;
-		__copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr);
+		if (__copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr)) {
+			kfree(cgc.buffer);
+			return -EFAULT;
+		}
 		ubuf += CD_FRAMESIZE_RAW * nr;
 		nframes -= nr;
 		lba += nr;



I'm wondering if it would make sense to wrap this branch in unlikely() 
since it should rarely fail...?
I should also mention that I've only compile tested this so far.


--
Jesper Juhl

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Mon, Sep 06 2004, Jesper Juhl wrote:
> 
> Hi,
> 
> Here's a patch to ensure that the return value from __copy_to_user() gets 
> checked in cdrom_read_cdda_old().
> I assume that returning -EFAULT if the copy fails to copy all bytes is an 
> appropriate action, but please correct me if I'm wrong.
> 
> Signed-off-by: Jesper Juhl <juhl-lkml@dif.dk>
> 
> diff -up linux-2.6.9-rc1-bk13-orig/drivers/cdrom/cdrom.c linux-2.6.9-rc1-bk13/drivers/cdrom/cdrom.c
> --- linux-2.6.9-rc1-bk13-orig/drivers/cdrom/cdrom.c	2004-08-24 20:44:01.000000000 +0200
> +++ linux-2.6.9-rc1-bk13/drivers/cdrom/cdrom.c	2004-09-06 23:41:20.000000000 +0200
> @@ -1959,7 +1959,10 @@ static int cdrom_read_cdda_old(struct cd
>  		ret = cdrom_read_block(cdi, &cgc, lba, nr, 1, CD_FRAMESIZE_RAW);
>  		if (ret)
>  			break;
> -		__copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr);
> +		if (__copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr)) {
> +			kfree(cgc.buffer);
> +			return -EFAULT;
> +		}
>  		ubuf += CD_FRAMESIZE_RAW * nr;
>  		nframes -= nr;
>  		lba += nr;

__copy_to_user is the unchecking version of copy_to_user.

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Paul Mackerras]

Jens Axboe writes:

> __copy_to_user is the unchecking version of copy_to_user.

It doesn't range-check the address, but it does return non-zero
(number of bytes not copied) if it encounters a fault writing to the
user buffer.

Paul.

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, Paul Mackerras wrote:
> Jens Axboe writes:
> 
> > __copy_to_user is the unchecking version of copy_to_user.
> 
> It doesn't range-check the address, but it does return non-zero
> (number of bytes not copied) if it encounters a fault writing to the
> user buffer.

but it doesn't matter, if it returns non-zero then something happened
between the access_ok() and the actual copy because the user app did
something silly. so I don't care much really, I think the major point is
the kernel will cope.

you could remove the access_ok() and change it to a copy_to_user()
instead, I don't care either way. it's the old and slow interface which
really never is used unless things have gone wrong anyways.

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Andrew Morton]

Jens Axboe <axboe@suse.de> wrote:
>
> On Tue, Sep 07 2004, Paul Mackerras wrote:
> > Jens Axboe writes:
> > 
> > > __copy_to_user is the unchecking version of copy_to_user.
> > 
> > It doesn't range-check the address, but it does return non-zero
> > (number of bytes not copied) if it encounters a fault writing to the
> > user buffer.
> 
> but it doesn't matter, if it returns non-zero then something happened
> between the access_ok() and the actual copy because the user app did
> something silly. so I don't care much really, I think the major point is
> the kernel will cope.
> 
> you could remove the access_ok() and change it to a copy_to_user()
> instead, I don't care either way. it's the old and slow interface which
> really never is used unless things have gone wrong anyways.
> 

Sure, but at present if an application tries to read cdrom data to address
0x00000000 (say), the kernel will return "success".  It should return an
error code.  (Actually, it should return a short read if any data was
transferred, but whatever).

Plus the patch will fix a __must_check warning.

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, Andrew Morton wrote:
> Jens Axboe <axboe@suse.de> wrote:
> >
> > On Tue, Sep 07 2004, Paul Mackerras wrote:
> > > Jens Axboe writes:
> > > 
> > > > __copy_to_user is the unchecking version of copy_to_user.
> > > 
> > > It doesn't range-check the address, but it does return non-zero
> > > (number of bytes not copied) if it encounters a fault writing to the
> > > user buffer.
> > 
> > but it doesn't matter, if it returns non-zero then something happened
> > between the access_ok() and the actual copy because the user app did
> > something silly. so I don't care much really, I think the major point is
> > the kernel will cope.
> > 
> > you could remove the access_ok() and change it to a copy_to_user()
> > instead, I don't care either way. it's the old and slow interface which
> > really never is used unless things have gone wrong anyways.
> > 
> 
> Sure, but at present if an application tries to read cdrom data to address
> 0x00000000 (say), the kernel will return "success".  It should return an
> error code.  (Actually, it should return a short read if any data was
> transferred, but whatever).

Because access_ok() isn't reliable? Otherwise I don't see how that will
happen. There is another bug in there though, ret is never returned if
cdrom_read_block() fails.

> Plus the patch will fix a __must_check warning.

Then lets do it right.

===== drivers/cdrom/cdrom.c 1.69 vs edited =====
--- 1.69/drivers/cdrom/cdrom.c	2004-08-23 10:15:20 +02:00
+++ edited/drivers/cdrom/cdrom.c	2004-09-07 12:08:13 +02:00
@@ -1946,11 +1946,6 @@
 	if (!nr)
 		return -ENOMEM;
 
-	if (!access_ok(VERIFY_WRITE, ubuf, nframes * CD_FRAMESIZE_RAW)) {
-		kfree(cgc.buffer);
-		return -EFAULT;
-	}
-
 	cgc.data_direction = CGC_DATA_READ;
 	while (nframes > 0) {
 		if (nr > nframes)
@@ -1959,13 +1954,16 @@
 		ret = cdrom_read_block(cdi, &cgc, lba, nr, 1, CD_FRAMESIZE_RAW);
 		if (ret)
 			break;
-		__copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr);
+		ret = -EFAULT;
+		if (copy_to_user(ubuf, cgc.buffer, CD_FRAMESIZE_RAW * nr))
+			break;
 		ubuf += CD_FRAMESIZE_RAW * nr;
 		nframes -= nr;
 		lba += nr;
+		ret = 0;
 	}
 	kfree(cgc.buffer);
-	return 0;
+	return ret;
 }
 
 static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Andrew Morton]

Jens Axboe <axboe@suse.de> wrote:
>
> On Tue, Sep 07 2004, Andrew Morton wrote:
> > Jens Axboe <axboe@suse.de> wrote:
> > >
> > > On Tue, Sep 07 2004, Paul Mackerras wrote:
> > > > Jens Axboe writes:
> > > > 
> > > > > __copy_to_user is the unchecking version of copy_to_user.
> > > > 
> > > > It doesn't range-check the address, but it does return non-zero
> > > > (number of bytes not copied) if it encounters a fault writing to the
> > > > user buffer.
> > > 
> > > but it doesn't matter, if it returns non-zero then something happened
> > > between the access_ok() and the actual copy because the user app did
> > > something silly. so I don't care much really, I think the major point is
> > > the kernel will cope.
> > > 
> > > you could remove the access_ok() and change it to a copy_to_user()
> > > instead, I don't care either way. it's the old and slow interface which
> > > really never is used unless things have gone wrong anyways.
> > > 
> > 
> > Sure, but at present if an application tries to read cdrom data to address
> > 0x00000000 (say), the kernel will return "success".  It should return an
> > error code.  (Actually, it should return a short read if any data was
> > transferred, but whatever).
> 
> Because access_ok() isn't reliable?

access_ok() simply checks that the address is in the 0x00000000 -
0xbfffffff range.  We can still get faults in that range.


> There is another bug in there though, ret is never returned if
> cdrom_read_block() fails.

yup.

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, Andrew Morton wrote:
> Jens Axboe <axboe@suse.de> wrote:
> >
> > On Tue, Sep 07 2004, Andrew Morton wrote:
> > > Jens Axboe <axboe@suse.de> wrote:
> > > >
> > > > On Tue, Sep 07 2004, Paul Mackerras wrote:
> > > > > Jens Axboe writes:
> > > > > 
> > > > > > __copy_to_user is the unchecking version of copy_to_user.
> > > > > 
> > > > > It doesn't range-check the address, but it does return non-zero
> > > > > (number of bytes not copied) if it encounters a fault writing to the
> > > > > user buffer.
> > > > 
> > > > but it doesn't matter, if it returns non-zero then something happened
> > > > between the access_ok() and the actual copy because the user app did
> > > > something silly. so I don't care much really, I think the major point is
> > > > the kernel will cope.
> > > > 
> > > > you could remove the access_ok() and change it to a copy_to_user()
> > > > instead, I don't care either way. it's the old and slow interface which
> > > > really never is used unless things have gone wrong anyways.
> > > > 
> > > 
> > > Sure, but at present if an application tries to read cdrom data to address
> > > 0x00000000 (say), the kernel will return "success".  It should return an
> > > error code.  (Actually, it should return a short read if any data was
> > > transferred, but whatever).
> > 
> > Because access_ok() isn't reliable?
> 
> access_ok() simply checks that the address is in the 0x00000000 -
> 0xbfffffff range.  We can still get faults in that range.

Ok, it's pretty useless then.

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[viro]

On Tue, Sep 07, 2004 at 11:34:37AM +0200, Jens Axboe wrote:
> On Tue, Sep 07 2004, Paul Mackerras wrote:
> > Jens Axboe writes:
> > 
> > > __copy_to_user is the unchecking version of copy_to_user.
> > 
> > It doesn't range-check the address, but it does return non-zero
> > (number of bytes not copied) if it encounters a fault writing to the
> > user buffer.
> 
> but it doesn't matter, if it returns non-zero then something happened
> between the access_ok() and the actual copy because the user app did
> something silly. so I don't care much really, I think the major point is
> the kernel will cope.
> 
> you could remove the access_ok() and change it to a copy_to_user()
> instead, I don't care either way. it's the old and slow interface which
> really never is used unless things have gone wrong anyways.

access_ok() is far from being the only reason for error here.  If area
is unmapped, we shouldn't silently lose data without any indication of
error.

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Tue, Sep 07, 2004 at 11:34:37AM +0200, Jens Axboe wrote:
> > On Tue, Sep 07 2004, Paul Mackerras wrote:
> > > Jens Axboe writes:
> > > 
> > > > __copy_to_user is the unchecking version of copy_to_user.
> > > 
> > > It doesn't range-check the address, but it does return non-zero
> > > (number of bytes not copied) if it encounters a fault writing to the
> > > user buffer.
> > 
> > but it doesn't matter, if it returns non-zero then something happened
> > between the access_ok() and the actual copy because the user app did
> > something silly. so I don't care much really, I think the major point is
> > the kernel will cope.
> > 
> > you could remove the access_ok() and change it to a copy_to_user()
> > instead, I don't care either way. it's the old and slow interface which
> > really never is used unless things have gone wrong anyways.
> 
> access_ok() is far from being the only reason for error here.  If area
> is unmapped, we shouldn't silently lose data without any indication of
> error.

it boils down to access_ok() not being sufficient on its own, and in
which case yes we should just use copy_to_user() and kill the check
completely as per the patch sent out.

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[viro]

On Tue, Sep 07, 2004 at 12:30:31PM +0200, Jens Axboe wrote:
> it boils down to access_ok() not being sufficient on its own, and in
> which case yes we should just use copy_to_user() and kill the check
> completely as per the patch sent out.

access_ok() is just "we can trust MMU to do the right thing when dealing
with access to process address space at that address".  On platforms with
secondary address spaces (e.g. sparc) it's always true.  On something like
i386 we *could* use segments for the same purposes.  In fact, we used to
do just that - access to userland memory went with %fs as segment (thus
the names like extinct memcpy_fromfs() and surviving set_fs()).  However,
it's cheaper to do that check explicitly instead of relying on MMU.  And
that's what access_ok() does.

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, viro@parcelfarce.linux.theplanet.co.uk wrote:
> On Tue, Sep 07, 2004 at 12:30:31PM +0200, Jens Axboe wrote:
> > it boils down to access_ok() not being sufficient on its own, and in
> > which case yes we should just use copy_to_user() and kill the check
> > completely as per the patch sent out.
> 
> access_ok() is just "we can trust MMU to do the right thing when dealing
> with access to process address space at that address".  On platforms with
> secondary address spaces (e.g. sparc) it's always true.  On something like
> i386 we *could* use segments for the same purposes.  In fact, we used to
> do just that - access to userland memory went with %fs as segment (thus
> the names like extinct memcpy_fromfs() and surviving set_fs()).  However,
> it's cheaper to do that check explicitly instead of relying on MMU.  And
> that's what access_ok() does.

Alright, I'm wondering how the misconception of what access_ok() really
guarantees snuck into cdrom.c. At least the patch takes care of it.

-- 
Jens Axboe

Re: [PATCH] remember to check return value from __copy_to_user() in cdrom_read_cdda_old()

[Jens Axboe]

On Tue, Sep 07 2004, Paul Mackerras wrote:
> Jens Axboe writes:
> 
> > __copy_to_user is the unchecking version of copy_to_user.
> 
> It doesn't range-check the address, but it does return non-zero
> (number of bytes not copied) if it encounters a fault writing to the
> user buffer.

I don't see your point. We already access checked the range, so if
__copy_to_user() returns non-zero, the application is buggy.

-- 
Jens Axboe