Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753241AbcD0QWx (ORCPT ); Wed, 27 Apr 2016 12:22:53 -0400 Received: from mail-wm0-f52.google.com ([74.125.82.52]:37564 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752287AbcD0QWu (ORCPT ); Wed, 27 Apr 2016 12:22:50 -0400 MIME-Version: 1.0 In-Reply-To: References: <0484FFD3-4BAB-43B9-AD56-B4A098C3E8AE@gmail.com> <20160427080105.GI17913@mwanda> Date: Wed, 27 Apr 2016 09:22:48 -0700 Message-ID: Subject: Re: Double-Fetch bug in Linux-4.5/drivers/scsi/aacraid/commctrl.c From: Kees Cook To: Julia Lawall Cc: Dan Carpenter , Pengfei Wang , "security@kernel.org" , LKML Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2610 Lines: 74 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