From: Julia Lawall <julia.lawall@lip6.fr>
To: Kees Cook <keescook@chromium.org>
Cc: linux-kernel@vger.kernel.org,
Gilles Muller <Gilles.Muller@lip6.fr>,
Nicolas Palix <nicolas.palix@imag.fr>,
Michal Marek <mmarek@suse.com>,
Pengfei Wang <wpengfeinudt@gmail.com>,
cocci@systeme.lip6.fr, Vaishali Thakkar <vthakkar1994@gmail.com>
Subject: Re: [PATCH] coccicheck: add a test for repeat copy_from_user
Date: Tue, 27 Dec 2016 19:21:56 +0100 (CET) [thread overview]
Message-ID: <alpine.DEB.2.20.1612271916250.2001@hadrien> (raw)
In-Reply-To: <20160426222442.GA8104@www.outflux.net>
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 <keescook@chromium.org>
> ---
> 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
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.
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
>
next prev parent reply other threads:[~2016-12-27 18:22 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-26 22:24 [PATCH] coccicheck: add a test for repeat copy_from_user Kees Cook
2016-04-26 22:30 ` Kees Cook
2016-12-27 18:21 ` Julia Lawall [this message]
2017-01-09 17:05 ` [Cocci] " Vaishali Thakkar
2017-01-09 19:08 ` Julia Lawall
2017-01-09 20:56 ` Kees Cook
2017-01-09 22:02 ` Kees Cook
[not found] ` <05AE3A59-EF48-4FFF-A028-0204B2E56DEB@gmail.com>
2017-01-10 8:40 ` Vaishali Thakkar
[not found] ` <19545870-5238-4BEB-AF1E-741BA97A6AA2@gmail.com>
2017-01-10 17:46 ` Vaishali Thakkar
[not found] ` <76D088EA-3C7E-4766-A237-3FA1F0767C1A@gmail.com>
2017-01-11 6:12 ` Julia Lawall
2017-01-11 13:44 ` Pengfei Wang
2017-01-10 19:16 ` Kees Cook
2017-01-10 19:15 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=alpine.DEB.2.20.1612271916250.2001@hadrien \
--to=julia.lawall@lip6.fr \
--cc=Gilles.Muller@lip6.fr \
--cc=cocci@systeme.lip6.fr \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mmarek@suse.com \
--cc=nicolas.palix@imag.fr \
--cc=vthakkar1994@gmail.com \
--cc=wpengfeinudt@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).