2003-05-25 16:12:19

by Paulo Andre'

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

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'




Attachments:
patch-scsi_ioctl.c.diff (1.22 kB)

2003-05-25 16:15:31

by Jens Axboe

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

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

2003-05-25 16:42:30

by Oliver Neukum

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

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

2003-05-25 16:44:11

by Paulo Andre'

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

On Sun, 25 May 2003 18:28:44 +0200
Jens Axboe <[email protected]> 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


Attachments:
patch-scsi_ioctl.c.diff (941.00 B)

2003-05-25 18:38:59

by dan carpenter

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

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