linux-kernel - Re: [PATCH v4] coccinelle: tests: unsigned value cannot be lesser than zero

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.02.1512301227090.2048@localhost6.localdomain6>
Date:	Wed, 30 Dec 2015 12:29:13 +0100 (CET)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Andrzej Hajda <a.hajda@...sung.com>
cc:	Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
	Marek Szyprowski <m.szyprowski@...sung.com>,
	Gilles Muller <Gilles.Muller@...6.fr>,
	Nicolas Palix <nicolas.palix@...g.fr>,
	Michal Marek <mmarek@...e.com>,
	open list <linux-kernel@...r.kernel.org>,
	"moderated list:COCCINELLE/Semantic Patches (SmPL)" 
	<cocci@...teme.lip6.fr>
Subject: Re: [PATCH v4] coccinelle: tests: unsigned value cannot be lesser
 than zero



On Wed, 30 Dec 2015, Andrzej Hajda wrote:

> Unsigned expressions cannot be lesser than zero. Presence of comparisons
> 'unsigned (<|<=|>|>=) 0' often indicates a bug, usually wrong type of variable.
> The patch beside finding such comparisons tries to eliminate false positives,
> mainly by bypassing range checks.
> 
> gcc can detect such comparisons also using -Wtype-limits switch, but it warns
> also in correct cases, making too much noise.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@...sung.com>
> ---
> v4: added range check detection, added full check in case value holds a result
>     of signed function
> v3: added bool type
> v2: added --all-includes option
> ---
> Hi Julia,
> 
> This version adds range check detection, as a result false positives are almost
> fully eliminated. Most of kernel patches have been already sent and accepted,
> but some new bugs appeared since then. I will prepare bugfixes soon.
> 
> I have enountered one issue, the patch does not detect range check in
> drivers/leds/leds-tca6507.c:716:
>     if (ret != 0 || reg < 0 || reg >= NUM_LEDS)
> 
> Simplified check, responsible for detectin range checks:
> @@
> expression v, c;
> @@
> 
> * (\( v < 0 \| v <= 0 \)) || (\( v >= c \| v > c \))
> 
> Is it a bug or expected behavior? Maybe consequence of left-to-right associativity?

Yes, it would be an associativity problem.  Could you try with || ... 
added to the right end of your pattern?  That should allow it to let the 
disjunctions appear anywhere at top level, but I'm not sure to what extent 
it works when the pattern already contains a disjunction.  You could also 
try

A || ... || B || ...

julia

> 
> Regards
> Andrzej
> 
>  .../tests/unsigned_lesser_than_zero.cocci          | 77 ++++++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci
> 
> diff --git a/scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci b/scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci
> new file mode 100644
> index 0000000..e977447
> --- /dev/null
> +++ b/scripts/coccinelle/tests/unsigned_lesser_than_zero.cocci
> @@ -0,0 +1,77 @@
> +/// Unsigned expressions cannot be lesser than zero. Presence of
> +/// comparisons 'unsigned (<|<=|>|>=) 0' often indicates a bug,
> +/// usually wrong type of variable.
> +///
> +/// To reduce number of false positives following tests have been added:
> +/// - parts of range checks are skipped, eg. "if (u < 0 || u > 15) ...",
> +///   developers prefer to keep such code,
> +/// - comparisons "<= 0" and "> 0" are performed only on results of
> +///   signed functions/macros,
> +/// - hardcoded list of signed functions/macros with always non-negative
> +///   result is used to avoid false positives difficult to detect by other ways
> +///
> +// Confidence: Average
> +// Copyright: (C) 2015 Andrzej Hajda, Samsung Electronics Co., Ltd. GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Options: --all-includes
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +@...mp@
> +position p;
> +typedef bool, u8, u16, u32, u64;
> +{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long,
> +	size_t, bool, u8, u16, u32, u64} v;
> +expression e;
> +@@
> +	\( v = e \| &v \)
> +	...
> +	(\( v@p < 0 \| v@p <= 0 \| v@p >= 0 \| v@p > 0 \))
> +
> +@r@
> +position r_cmp.p;
> +typedef s8, s16, s32, s64;
> +{char, short, int, long, long long, ssize_t, s8, s16, s32, s64} vs;
> +expression c, e, v;
> +identifier f !~ "^(ata_id_queue_depth|btrfs_copy_from_user|dma_map_sg|dma_map_sg_attrs|fls|fls64|gameport_time|get_write_extents|nla_len|ntoh24|of_flat_dt_match|of_get_child_count|uart_circ_chars_pending|[A-Z0-9_]+)$";
> +@@
> +
> +(
> +	...
> +(
> +	(\( v@p < 0 \| v@p <= 0 \)) || (\( v >= c \| v > c \))
> +|
> +	(\( v >= c \| v > c \)) || (\( v@p < 0 \| v@p <= 0 \))
> +|
> +	(\( v@p >= 0 \| v@p > 0 \)) && (\( v < c \| v <= c \))
> +|
> +	((\( v < c \| v <= c \) && \( v@p >= 0 \| v@p > 0 \)))
> +|
> +*	(\( v@p <@e 0 \| v@p >=@e 0 \))
> +)
> +	...
> +|
> +	v = f(...)@vs;
> +	... when != v = e;
> +*	(\( v@p <=@e 0 \| v@p >@e 0 \))
> +	...
> +)
> +
> +@...ipt:python depends on org@
> +p << r_cmp.p;
> +e << r.e = "";
> +@@
> +
> +msg = "WARNING: Unsigned expression compared with zero: %s" % (e)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@...ipt:python depends on report@
> +p << r_cmp.p;
> +e << r.e = "";
> +@@
> +
> +msg = "WARNING: Unsigned expression compared with zero: %s" % (e)
> +if e:
> +    coccilib.report.print_report(p[0], msg)
> -- 
> 1.9.1
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/