From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1763015AbdAIRFl (ORCPT ); Mon, 9 Jan 2017 12:05:41 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:24169 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1762967AbdAIRFj (ORCPT ); Mon, 9 Jan 2017 12:05:39 -0500 Subject: Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user To: Julia Lawall , Kees Cook References: <20160426222442.GA8104@www.outflux.net> Cc: Pengfei Wang , Vaishali Thakkar , linux-kernel@vger.kernel.org, Michal Marek , cocci@systeme.lip6.fr From: Vaishali Thakkar Message-ID: Date: Mon, 9 Jan 2017 22:35:19 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: aserv0021.oracle.com [141.146.126.233] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > >> This is usually a sign of a resized request. This adds a check for >> potential races or confusions. The check isn't 100% accurate, so it >> needs some manual review. >> >> Signed-off-by: Kees Cook >> --- >> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> >> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >> new file mode 100644 >> index 000000000000..53645de8ae95 >> --- /dev/null >> +++ b/scripts/coccinelle/tests/reusercopy.cocci >> @@ -0,0 +1,36 @@ >> +/// Recopying from the same user buffer frequently indicates a pattern of >> +/// Reading a size header, allocating, and then re-reading an entire >> +/// structure. If the structure's size is not re-validated, this can lead >> +/// to structure or data size confusions. >> +/// >> +// Confidence: Moderate >> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> +// URL: http://coccinelle.lip6.fr/ >> +// Comments: >> +// Options: -no_includes -include_headers > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > >> + >> +virtual report >> +virtual org > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user@p(&dest, src, sizeof(dest)) > >> + >> +@cfu_twice@ >> +position p; > > Change this to: > > position p != ok.p; > >> +identifier src; >> +expression dest1, dest2, size1, size2, offset; >> +@@ >> + >> +*copy_from_user(dest1, src, size1) >> + ... when != src = offset >> + when != src += offset Here, may be we should add few more lines from Pengfei's script to avoid th potential FPs. > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > thanks, > julia > >> +*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()") >> -- >> 2.6.3 >> >> >> -- >> Kees Cook >> Chrome OS & Brillo Security >> > _______________________________________________ > Cocci mailing list > Cocci@systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci > From mboxrd@z Thu Jan 1 00:00:00 1970 From: vaishali.thakkar@oracle.com (Vaishali Thakkar) Date: Mon, 9 Jan 2017 22:35:19 +0530 Subject: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user In-Reply-To: References: <20160426222442.GA8104@www.outflux.net> Message-ID: To: cocci@systeme.lip6.fr List-Id: cocci@systeme.lip6.fr On Tuesday 27 December 2016 11:51 PM, Julia Lawall wrote: > I totally dropped the ball on this. Many thanks to Vaishali for > resurrecting it. > > Some changes are suggested below. > > On Tue, 26 Apr 2016, Kees Cook wrote: > >> This is usually a sign of a resized request. This adds a check for >> potential races or confusions. The check isn't 100% accurate, so it >> needs some manual review. >> >> Signed-off-by: Kees Cook >> --- >> scripts/coccinelle/tests/reusercopy.cocci | 36 +++++++++++++++++++++++++++++++ >> 1 file changed, 36 insertions(+) >> create mode 100644 scripts/coccinelle/tests/reusercopy.cocci >> >> diff --git a/scripts/coccinelle/tests/reusercopy.cocci b/scripts/coccinelle/tests/reusercopy.cocci >> new file mode 100644 >> index 000000000000..53645de8ae95 >> --- /dev/null >> +++ b/scripts/coccinelle/tests/reusercopy.cocci >> @@ -0,0 +1,36 @@ >> +/// Recopying from the same user buffer frequently indicates a pattern of >> +/// Reading a size header, allocating, and then re-reading an entire >> +/// structure. If the structure's size is not re-validated, this can lead >> +/// to structure or data size confusions. >> +/// >> +// Confidence: Moderate >> +// Copyright: (C) 2016 Kees Cook, Google. License: GPLv2. >> +// URL: http://coccinelle.lip6.fr/ >> +// Comments: >> +// Options: -no_includes -include_headers > > The options could be: --no-include --include-headers > > Actually, Coccinelle supports both, but it only officially supports the > -- versions. > >> + >> +virtual report >> +virtual org > > Add, the following for the *s: > > virtual context > > Then add the following rule: > > @ok@ > position p; > expression src,dest; > @@ > > copy_from_user at p(&dest, src, sizeof(dest)) > >> + >> + at cfu_twice@ >> +position p; > > Change this to: > > position p != ok.p; > >> +identifier src; >> +expression dest1, dest2, size1, size2, offset; >> +@@ >> + >> +*copy_from_user(dest1, src, size1) >> + ... when != src = offset >> + when != src += offset Here, may be we should add few more lines from Pengfei's script to avoid th potential FPs. > Add the following lines: > > when != if (size2 > e1 || ...) { ... return ...; } > when != if (size2 > e1 || ...) { ... size2 = e2 ... } > > These changes drop cases where the last argument to copy_from_usr is the > size of the first argument, which seems safe enough, and where there is a > test on the size value that can either update it or abort the function. > These changes only eliminate false positives, as far as I could tell. > > If it would be more convenient, I could just send the complete revised > patch, or whatever seems convenient. I was also thinking that probably we should also add other user space memory API functions. May be get_user and strncpy_from_user. Although I'm not sure how common it is to find such patterns for both of these functions. > thanks, > julia > >> +*copy_from_user at p(dest2, src, size2) >> + >> + at script:python depends on org@ >> +p << cfu_twice.p; >> +@@ >> + >> +cocci.print_main("potentially dangerous second copy_from_user()",p) >> + >> + at script:python depends on report@ >> +p << cfu_twice.p; >> +@@ >> + >> +coccilib.report.print_report(p[0],"potentially dangerous second copy_from_user()") >> -- >> 2.6.3 >> >> >> -- >> Kees Cook >> Chrome OS & Brillo Security >> > _______________________________________________ > Cocci mailing list > Cocci at systeme.lip6.fr > https://systeme.lip6.fr/mailman/listinfo/cocci >