Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c
On Wed, Apr 27, 2016 at 1:07 AM, Julia Lawall wrote: > > > On Wed, 27 Apr 2016, Dan Carpenter wrote: > >> On Wed, Apr 27, 2016 at 07:42:04AM +0200, Julia Lawall wrote: >> > >> > >> > On Tue, 26 Apr 2016, Kees Cook wrote: >> > >> > > On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang >> > > 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? >> > >> >> >> copy_from_user(dest, src, sizeof(dest)); >> >> if (dest.extra > MAX_SIZE) >> return -EINVAL; >> >> copy_from_user(dest, src, sizeof(dest) + dest.extra); >> >> for (i = 0; i < dest.extra; i++) { >> dest.foo[i] = xxx; >> >> >> We get dest.extra from the user, we verify the size, then we copy more >> data from the user but that over writes dest.extra again. We use >> dest.extra a second time without checking that it's still <= MAX_SIZE. > > OK, so the problem is when data that was checked on the first copy is used > after the second copy? It would probably be possible to get rid of a lot > of false positives with that. Yeah, though sometimes it's not into the same structure/variable: copy_from_user(&header, src, sizeof(header)); full_structure = kmalloc(header.size); copy_from_user(full_structure, src, header.size); do_things(full_structure); copy_to_user(dest, full_structure, full_structure->size); Dan's example is the worst-case, but my above example can lead to under-reads, or otherwise confusing actions taken when examining full_structures's "size" field vs what has actually be written, etc. (In my example, do_things may operate on uninitialize fields in full_structure, and will leak heap contents on the copy_to_user.) As a result of these variations, I was just detecting a double read from the same location, which is usually an indication of some kind of confusion in the code. -Kees -- Kees Cook Chrome OS & Brillo Security
Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c
On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang 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
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 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 >
Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c
On Wed, Apr 27, 2016 at 07:42:04AM +0200, Julia Lawall wrote: > > > On Tue, 26 Apr 2016, Kees Cook wrote: > > > On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang > > 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? > copy_from_user(dest, src, sizeof(dest)); if (dest.extra > MAX_SIZE) return -EINVAL; copy_from_user(dest, src, sizeof(dest) + dest.extra); for (i = 0; i < dest.extra; i++) { dest.foo[i] = xxx; We get dest.extra from the user, we verify the size, then we copy more data from the user but that over writes dest.extra again. We use dest.extra a second time without checking that it's still <= MAX_SIZE. regards, dan carpenter
Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c
On Wed, 27 Apr 2016, Dan Carpenter wrote: > On Wed, Apr 27, 2016 at 07:42:04AM +0200, Julia Lawall wrote: > > > > > > On Tue, 26 Apr 2016, Kees Cook wrote: > > > > > On Mon, Apr 25, 2016 at 7:50 AM, Pengfei Wang > > > 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? > > > > > copy_from_user(dest, src, sizeof(dest)); > > if (dest.extra > MAX_SIZE) > return -EINVAL; > > copy_from_user(dest, src, sizeof(dest) + dest.extra); > > for (i = 0; i < dest.extra; i++) { > dest.foo[i] = xxx; > > > We get dest.extra from the user, we verify the size, then we copy more > data from the user but that over writes dest.extra again. We use > dest.extra a second time without checking that it's still <= MAX_SIZE. OK, so the problem is when data that was checked on the first copy is used after the second copy? It would probably be possible to get rid of a lot of false positives with that. thanks, julia