Alright, I got a lot further now adding "okay" states to my copy_from_user
check, but I'm stumped again.
I want this to be ignored:
len = min_t(unsigned int, sizeof(opts), optlen);
if (copy_from_user((char *) &opts, optval, len)) {
but some imaginary code like this to stand out:
len = min_t(unsigned int, sizeof(oops_something_else), optlen);
if (copy_from_user((char *) &opts, optval, len)) {
I'm able to do an inverted test for it (i.e. find the first style) with
this:
@cfu_min@
expression f;
identifier s, l;
type T1, T2;
T1 i;
@@
(
s = min(sizeof(i), l);
|
s = min_t(T2, sizeof(i), l);
)
... when != s
when != i
* copy_from_user((char*)&i, f, s)
However, when I add this to my other set of rules, it starts to accept the
second style of code, and I can't figure out what has gone wrong.
I've attached the before/after versions of my ever-grow cocci file. With a
modified (see "BADopts" below) net/bluetooth/l2cap.c to show as an example:
# Original ruleset, doesn't know that the added min() test is safe
$ spatch -cocci_file copy_from_user.cocci net/bluetooth/l2cap.c
init_defs_builtins: /usr/share/coccinelle/standard.h
HANDLING: net/bluetooth/l2cap.c
diff =
--- net/bluetooth/l2cap.c 2010-10-08 16:23:56.624893750 -0700
+++ /tmp/cocci-output-22478-65d33a-l2cap.c 2010-10-08 16:27:06.355269624 -0700
@@ -1969,7 +1969,6 @@ static int l2cap_sock_setsockopt_old(str
opts.txwin_size = (__u16)l2cap_pi(sk)->tx_win;
len = min_t(unsigned int, sizeof(BADopts), optlen);
- if (copy_from_user((char *) &opts, optval, len)) {
err = -EFAULT;
break;
}
@@ -2055,7 +2054,6 @@ static int l2cap_sock_setsockopt(struct
sec.level = BT_SECURITY_LOW;
len = min_t(unsigned int, sizeof(sec), optlen);
- if (copy_from_user((char *) &sec, optval, len)) {
err = -EFAULT;
break;
}
# New rule to detect min() test, shows a good one, ignores bad use of min()
$ spatch -cocci_file show-min-copy_from_user.cocci net/bluetooth/l2cap.c
init_defs_builtins: /usr/share/coccinelle/standard.h
HANDLING: net/bluetooth/l2cap.c
diff =
--- net/bluetooth/l2cap.c 2010-10-08 16:23:56.624893750 -0700
+++ /tmp/cocci-output-22509-383a61-l2cap.c 2010-10-08 16:27:36.025324437 -0700
@@ -2055,7 +2055,6 @@ static int l2cap_sock_setsockopt(struct
sec.level = BT_SECURITY_LOW;
len = min_t(unsigned int, sizeof(sec), optlen);
- if (copy_from_user((char *) &sec, optval, len)) {
err = -EFAULT;
break;
}
# Merged and test inverted it misses the bad use of min()
$ spatch -cocci_file withmin-copy_from_user.cocci net/bluetooth/l2cap.c
init_defs_builtins: /usr/share/coccinelle/standard.h
HANDLING: net/bluetooth/l2cap.c
Clearly something is over-matching in the merged "withmin-copy_from_user.cocci"
but it's not obvious where it's gone wrong. :P
Any help would be appreciated. :)
-Kees
--
Kees Cook
Ubuntu Security Team
@cfu_simple@
expression f;
identifier e;
type T;
T *i;
@@
(
copy_from_user(&e, f, sizeof(e))
|
copy_from_user(e, f, sizeof(*e))
|
copy_from_user(i, f, sizeof(T))
)
@cfu_alloc@
expression f;
expression s;
expression E;
type T;
T *i;
@@
i =
(
kmalloc(s, ...)
|
kzalloc(s, ...)
|
kmalloc_track_caller(s, ...)
|
vmalloc(s, ...)
|
usb_alloc_coherent(E, s, ...)
)
...
copy_from_user(i, f, s)
@cfu_testp@
identifier s;
expression f;
type T;
T *i;
@@
(
if ( s != sizeof(T) || ... ) { ... return ...; }
|
if ( s > sizeof(T) || ... ) { ... return ...; }
|
if ( s >= sizeof(T) || ... ) { ... return ...; }
)
... when != s
when != i
copy_from_user(i, f, s)
@cfu_testa@
expression f;
expression E;
identifier s, i;
type T;
@@
T i[E];
...
(
if ( s != E || ... ) { ... return ...; }
|
if ( s > E || ... ) { ... return ...; }
|
if ( s >= E || ... ) { ... return ...; }
|
if ( s != sizeof(i) || ... ) { ... return ...; }
|
if ( s > sizeof(i) || ... ) { ... return ...; }
|
if ( s >= sizeof(i) || ... ) { ... return ...; }
)
... when != s
when != i
copy_from_user(i, f, s)
@depends on (!cfu_simple && !cfu_alloc && !cfu_testp && !cfu_testa)@
@@
* copy_from_user(...)
@cfu_simple@
expression f;
identifier e;
type T;
T *i;
@@
(
copy_from_user(&e, f, sizeof(e))
|
copy_from_user(e, f, sizeof(*e))
|
copy_from_user(i, f, sizeof(T))
)
@cfu_alloc@
expression f;
expression s;
expression E;
type T;
T *i;
@@
i =
(
kmalloc(s, ...)
|
kzalloc(s, ...)
|
kmalloc_track_caller(s, ...)
|
vmalloc(s, ...)
|
usb_alloc_coherent(E, s, ...)
)
...
copy_from_user(i, f, s)
@cfu_testp@
identifier s;
expression f;
type T;
T *i;
@@
(
if ( s != sizeof(T) || ... ) { ... return ...; }
|
if ( s > sizeof(T) || ... ) { ... return ...; }
|
if ( s >= sizeof(T) || ... ) { ... return ...; }
)
... when != s
when != i
copy_from_user(i, f, s)
@cfu_testa@
expression f;
expression E;
identifier s, i;
type T;
@@
T i[E];
...
(
if ( s != E || ... ) { ... return ...; }
|
if ( s > E || ... ) { ... return ...; }
|
if ( s >= E || ... ) { ... return ...; }
|
if ( s != sizeof(i) || ... ) { ... return ...; }
|
if ( s > sizeof(i) || ... ) { ... return ...; }
|
if ( s >= sizeof(i) || ... ) { ... return ...; }
)
... when != s
when != i
copy_from_user(i, f, s)
@cfu_min@
expression f;
identifier s, l;
type T1, T2;
T1 i;
@@
(
s = min(sizeof(i), l);
|
s = min_t(T2, sizeof(i), l);
)
... when != s
when != i
copy_from_user((char*)&i, f, s)
@depends on (!cfu_simple && !cfu_alloc && !cfu_testp && !cfu_testa && !cfu_min)@
@@
* copy_from_user(...)
@cfu_min@
expression f;
identifier s, l;
type T1, T2;
T1 i;
@@
(
s = min(sizeof(i), l);
|
s = min_t(T2, sizeof(i), l);
)
... when != s
when != i
* copy_from_user((char*)&i, f, s)
_______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)
