Re: [Cocci] [PATCH] coccicheck: add a test for repeat copy_from_user
From: Vaishali Thakkar
Date: Tue Jan 10 2017 - 12:47:06 EST
On Tuesday 10 January 2017 02:32 PM, Pengfei Wang wrote:
在 2017年1月10日，下午4:40，Vaishali Thakkar <vaishali.thakkar@xxxxxxxxxx> 写道：
On Tuesday 10 January 2017 01:51 PM, Pengfei Wang wrote:
在 2017年1月10日，上午1:05，Vaishali Thakkar <vaishali.thakkar@xxxxxxxxxx> 写道：
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 <keescook@xxxxxxxxxxxx>
---
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.
I strongly recommend you adding get_user() API , which is used pervasively
within the kernel just like copy_from user().
Sure. I have changed regetuser-wang.cocci from Kees's RFC patches to
include everything in the pattern matching rule. I'll send that as well.
In many situations, there is a combination use, get_user() copies first then
followed by a copy_from_user() copy. According to our investigation, this typical
situation works by get_user() firstly copying a field of a specific struct to check,
then copy_from_user() copies in the whole struct to use. Of course, the struct
field is fetch twice.
Do you mean that there is a problem when we have get_user() followed by copy_from_user()? Basically something like
this:
get_user(..., src.arg) //where src.arg = field of a structure
...
copy_from_user(..., src, ...) //where src is a whole structure
If that is the case then we would need to have one more new script
or rule for such kind of combinational patterns. Disjunction can
probably give FPs.
Yes, I’ve seen these cases when examining the source code. Actually, copying a field
first and then copying the whole struct is very common in the kernel especially the driver.
For example, when a struct (or a message as we call it) is variable length, the first copy is
used to check its size field, and allocate a kernel buffer based on it, then the second copy is
to copy the whole message also based on the size. There are also situations of the
variable type messages.
The reason that they use get_user() instead of copy_from_user() for the first copy is because
get_user() is defined as a macro, which works faster than a function call that copy_from_user() does
when copy simple data type such as char and int.
I see. If possible, can you point me to a code or actual bug
[reported by you or others] which has this kind of pattern
particularly?
I wrote a separate rule for the kind of pattern you have
described but I am not sure if this kind of code is suspicious.
Like you said, it is very common to use this pattern in drivers.
So may be suspicious one can have a specific pattern for this
combinational usage of get_user and copy_from_user.
Thanks.
Regards
Pengfei
Thanks!
Regards
Pengfei
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@xxxxxxxxxxxxxxx <mailto:Cocci@xxxxxxxxxxxxxxx> <mailto:Cocci@xxxxxxxxxxxxxxx <mailto:Cocci@xxxxxxxxxxxxxxx>>
https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci> <https://systeme.lip6.fr/mailman/listinfo/cocci <https://systeme.lip6.fr/mailman/listinfo/cocci>>