Messages in this thread | | | Date | Wed, 27 Apr 2016 07:42:04 +0200 (CEST) | From | Julia Lawall <> | Subject | Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c |
| |
On Tue, 26 Apr 2016, Kees Cook wrote:
> On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang <wpengfeinudt@gmail.com> wrote: > > Hello, > > > > I found this Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c > > when I was examining the source code. > > Thanks for these reports! I wrote a coccinelle script to find these, > but it requires some manual checking. For what it's worth, it found > your report as well: > > ./drivers/scsi/aacraid/commctrl.c:116:5-19: potentially dangerous > second copy_from_user() > > So I should probably get this added to the coccicheck run... Maybe it > can get some clean up from Julia. :)
I looked a bit at the results, and didn't see anything obvious. What is the problem, exactly, and what would be a characteristic of a false positive?
thanks, julia
> virtual report > virtual org > > @cfu_twice@ > position p; > identifier src; > expression dest1, dest2, size1, size2, offset; > @@ > > *copy_from_user(dest1, src, size1) > ... when != src = offset > when != src += offset > *copy_from_user@p(dest2, src, size2) > > @script:python depends on org@ > p << cfu_twice.p; > @@ > > cocci.print_main("potentially dangerous second copy_from_user()",p) > > @script:python depends on report@ > p << cfu_twice.p; > @@ > > coccilib.report.print_report(p[0],"potentially dangerous second > copy_from_user()") > > > It would be great to have some one go through all the reports to see > which are legit. I'll send separate emails with the patch for > coccicheck and the output. > > -Kees > > > > > In function ioctl_send_fib(), the driver fetches user space data by pointer > > arg via copy_from_user(), and this happens twice at line 81 and line 116 > > respectively. The first fetched value (stored in kfib) is used to get the > > header and calculate the size at line 90 so as to copy the whole message > > later at line 116, which means the copy size of the whole message is based > > on the old value that came from the first fetch. Besides, the whole message > > copied in the second fetch also contains the header. > > > > However, when the function processes the message after the second fetch at > > line 130, it uses kfib->header.Size that came from the second fetch, which > > might be different from the one came from the first fetch as well as > > calculated the size to copy the message from user space to driver. > > > > If the kfib->header.Size is modified by a user thread under race condition > > between the fetch operations, for example changing to a very large value, > > this will lead to over-boundary access or other serious consequences in > > function aac_fib_send(). > > > > I also reported this to bugzilla, > > https://bugzilla.kernel.org/show_bug.cgi?id=116751 > > > > I am expecting a reply to confirm this, thank you! > > > > > > > > > > > > Kind regards > > Pengfei > > > > > > -- > Kees Cook > Chrome OS & Brillo Security >
| |