The problem is that you are not doing anything to force it to be
considering the same copy_from_user in each rule. So your "depends on
..." in the last rules just considers whether these rules have ever
applied. To be sure to be talking about the same copy_from_user in each
case, use a position variable. Then the depend on will be evaluated
specific to that position.
You might find the -debug option useful for finding these kinds of
problems.
julia
On Fri, 8 Oct 2010, Kees Cook wrote:
> 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
>
_______________________________________________
Cocci mailing list
[email protected]
http://lists.diku.dk/mailman/listinfo/cocci
(Web access from inside DIKUs LAN only)
