On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang <wpengfein...@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. :) 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