Question on verify_area() and friends wrt

Question on verify_area() and friends wrt

[Paulo Andre']

Hi,

I've been taking care of auditing some return values for instances of
unchecked copy_*_user calls and I've come across one case that's
marked as a bug at kbugs.org which however doesn't seem like one to me.
The piece of code I'm referring can be found in
net/bluetooth/hci_core.c:436

if (!verify_area(VERIFY_WRITE, ptr, sizeof(ir) +
		(sizeof(struct inquiry_info) * ir.num_rsp))) {
    copy_to_user(ptr, &ir, sizeof(ir));
    ptr += sizeof(ir);
    copy_to_user(ptr, buf, sizeof(struct inquiry_info) * ir.num_rsp);	} else
    err = -EFAULT;

I'm presuming verify_area() does its job fine returning 0 if the memory
is valid and -EFAULT if not. Thus, given the exact check that's been
done, there seems indeed to exist no need to check each call to
copy_to_user() below. Or is there?

Thanks in advance,


		Paulo

Re: Question on verify_area() and friends wrt

[Christoph Hellwig]

On Sun, May 25, 2003 at 12:46:25PM +0100, Paulo Andre' wrote:
> if (!verify_area(VERIFY_WRITE, ptr, sizeof(ir) +
> 		(sizeof(struct inquiry_info) * ir.num_rsp))) {
>     copy_to_user(ptr, &ir, sizeof(ir));
>     ptr += sizeof(ir);
>     copy_to_user(ptr, buf, sizeof(struct inquiry_info) * ir.num_rsp);	} else
>     err = -EFAULT;
> 
> I'm presuming verify_area() does its job fine returning 0 if the memory
> is valid and -EFAULT if not. Thus, given the exact check that's been
> done, there seems indeed to exist no need to check each call to
> copy_to_user() below. Or is there?

verify_area only does some checks so you need to check the return value
from copy_to_user.  You could switch to __copy_to_user, though.

Re: Question on verify_area() and friends wrt

[Paulo Andre']

Thanks for your quick reply, Cristoph.

One more question arises though...

On Sun, 25 May 2003 13:07:06 +0100
Christoph Hellwig <hch@infradead.org> wrote:

> verify_area only does some checks so you need to check the return
> value from copy_to_user.  You could switch to __copy_to_user, though.

Why would __copy_to_user be a good choice? AFAIK, __copy_to_user does no
validy checks (as opposed to copy_to_user which does access_ok()) so,
considering verify_area() does only some checks, one could argue that
there's even less checking done if using __copy_to_user. Where am I
interpreting this wrong (as I certainly am) ?

Thanks again,


		Paulo

Re: Question on verify_area() and friends wrt

[dan carpenter]

On Sunday 25 May 2003 03:53 pm, Paulo Andre' wrote:
> Christoph Hellwig <hch@infradead.org> wrote:
> > verify_area only does some checks so you need to check the return
> > value from copy_to_user.  You could switch to __copy_to_user, though.
>
> Why would __copy_to_user be a good choice? AFAIK, __copy_to_user does no
> validy checks (as opposed to copy_to_user which does access_ok()) so,
> considering verify_area() does only some checks, one could argue that
> there's even less checking done if using __copy_to_user. Where am I
> interpreting this wrong (as I certainly am) ?
>

copy_to_user() does the equivelent of a verify_area().  __copy_to_user() 
doesn't 
make the verify_area() check.If a function is going to be making a lot of 
copies to 
the same area, it makes sense to just do one verify_area() and use 
__copy_to_user().
Both copy_to_user() and __copy_to_user() can fail even though the 
verify_area() 
checks pass.

In this case there is only one copy to each area so it doesn't really make
sense to use __copy_to_user().

My patch would look like this:

--- net/bluetooth/hci_core.c.orig       2003-05-25 00:25:16.000000000 +0200
+++ net/bluetooth/hci_core.c    2003-05-25 00:25:34.000000000 +0200
@@ -431,14 +431,14 @@
 
        BT_DBG("num_rsp %d", ir.num_rsp);
 
-       if (!verify_area(VERIFY_WRITE, ptr, sizeof(ir) + 
-                       (sizeof(struct inquiry_info) * ir.num_rsp))) {
-               copy_to_user(ptr, &ir, sizeof(ir));
-               ptr += sizeof(ir);
-               copy_to_user(ptr, buf, sizeof(struct inquiry_info) * 
ir.num_rsp);
-       } else 
+       if (copy_to_user(ptr, &ir, sizeof(ir))) {
                err = -EFAULT;
-
+               goto free:
+       }
+       ptr += sizeof(ir);
+       if (copy_to_user(ptr, buf, sizeof(struct inquiry_info) * ir.num_rsp))
+               err = -EFAULT;
+free:
        kfree(buf);
 
 done: