public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
* [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals
@ 2022-07-13 20:08 dmalcolm at gcc dot gnu.org
2022-07-14 21:09 ` [Bug analyzer/106284] " dmalcolm at gcc dot gnu.org
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-07-13 20:08 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106284
Bug ID: 106284
Summary: False positives from -Wanalyzer-tainted-array-index
with optimized conditionals
Product: gcc
Version: 12.0
Status: UNCONFIRMED
Severity: normal
Priority: P3
Component: analyzer
Assignee: dmalcolm at gcc dot gnu.org
Reporter: dmalcolm at gcc dot gnu.org
Target Milestone: ---
Consider:
#define LOWER_LIMIT 5
#define UPPER_LIMIT 10
static int arr[UPPER_LIMIT];
static int
called_by_test_1 (int iarg)
{
return arr[iarg]; /* { dg-warning "without bounds checking" } */
}
int __attribute__((tainted_args))
test_1 (unsigned long ularg)
{
return called_by_test_1 (ularg);
}
static int
called_by_test_2 (int iarg)
{
if (iarg < LOWER_LIMIT || iarg > UPPER_LIMIT)
return 0;
return arr[iarg]; /* { dg-bogus "bounds checking" "" { xfail *-*-* } } */
}
int __attribute__((tainted_args))
test_2 (unsigned long ularg)
{
return called_by_test_2 (ularg);
}
int __attribute__((tainted_args))
test_3 (int iarg)
{
if (iarg < LOWER_LIMIT || iarg > UPPER_LIMIT)
return 0;
return arr[iarg]; /* { dg-bogus "bounds checking" "" { xfail *-*-* } } */
}
int __attribute__((tainted_args))
test_4 (unsigned int uiarg)
{
if (uiarg > UPPER_LIMIT)
return 0;
return arr[uiarg]; /* { dg-bogus "bounds checking" } */
}
This gives false positives from -Wanalyzer-tainted-array-index at -O1 and above
at the xfails.
See: https://godbolt.org/z/ahq7G6b4n
Affects trunk and gcc 12.1; would possibly also affect 11 (but that reproducer
uses __attribute__((tainted_args)) which gcc 12 added).
Seen via false positive on Linux kernel in drivers/usb/class/usblp.c in
function ‘usblp_set_protocol’ handling usblp_ioctl on IOCNR_SET_PROTOCOL, which
has:
| 1337 | if (protocol < USBLP_FIRST_PROTOCOL || protocol >
USBLP_LAST_PROTOCOL)
| | ~
| | |
| | (15) following ‘false’ branch...
|......
| 1341 | if (usblp->intf->num_altsetting > 1) {
| | ~~~~~~~~~~~~
| | | |
| | | (16) ...to here
| | (17) following ‘true’ branch...
| 1342 | alts = usblp->protocol[protocol].alt_setting;
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (18) ...to here
| | (19) use of attacker-controlled value ‘arg’ in
array lookup without bounds checking
where "arg" is "protocol" (but from the caller frame), and is checked at (15).
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bug analyzer/106284] False positives from -Wanalyzer-tainted-array-index with optimized conditionals
2022-07-13 20:08 [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals dmalcolm at gcc dot gnu.org
@ 2022-07-14 21:09 ` dmalcolm at gcc dot gnu.org
2022-07-15 15:30 ` cvs-commit at gcc dot gnu.org
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-07-14 21:09 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106284
David Malcolm <dmalcolm at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Ever confirmed|0 |1
Status|UNCONFIRMED |ASSIGNED
Last reconfirmed| |2022-07-14
--- Comment #1 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
I'm testing a fix for this.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bug analyzer/106284] False positives from -Wanalyzer-tainted-array-index with optimized conditionals
2022-07-13 20:08 [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals dmalcolm at gcc dot gnu.org
2022-07-14 21:09 ` [Bug analyzer/106284] " dmalcolm at gcc dot gnu.org
@ 2022-07-15 15:30 ` cvs-commit at gcc dot gnu.org
2022-07-15 15:41 ` dmalcolm at gcc dot gnu.org
2022-07-19 13:56 ` cvs-commit at gcc dot gnu.org
3 siblings, 0 replies; 5+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-07-15 15:30 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106284
--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:
https://gcc.gnu.org/g:0a8edfbd37d399d1103d86e134ba0a92f8c873c3
commit r13-1713-g0a8edfbd37d399d1103d86e134ba0a92f8c873c3
Author: David Malcolm <dmalcolm@redhat.com>
Date: Fri Jul 15 11:28:34 2022 -0400
analyzer: fix taint false positive on optimized range checks [PR106284]
PR analyzer/106284 reports a false positive from
-Wanalyzer-tainted-array-index seen on the Linux kernel
with a version of my patches from:
https://gcc.gnu.org/pipermail/gcc-patches/2021-November/584372.html
in drivers/usb/class/usblp.c in function âusblp_set_protocolâ handling
usblp_ioctl on IOCNR_SET_PROTOCOL, which has:
| 1337 | if (protocol < USBLP_FIRST_PROTOCOL || protocol >
USBLP_LAST_PROTOCOL)
| | ~
| | |
| | (15) following âfalseâ branch...
|......
| 1341 | if (usblp->intf->num_altsetting > 1) {
| | ~~~~~~~~~~~~
| | | |
| | | (16) ...to here
| | (17) following âtrueâ branch...
| 1342 | alts = usblp->protocol[protocol].alt_setting;
| | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| | |
| | (18) ...to here
| | (19) use of attacker-controlled value
âargâ in array lookup without bounds checking
where "arg" is "protocol" (albeit from the caller frame, the ioctl
callback), and is clearly checked at (15).
The root cause is that at -O1 and above fold-const's build_range-check
can optimize range checks
(c>=low) && (c<=high)
into
(c-low>=0) && (c-low<=high-low)
and thus into a single check:
(unsigned)(c - low) <= (unsigned)(high-low).
I initially attempted to fix this by detecting such conditions in
region_model::on_condition, and calling on_condition for both of the
implied conditions. This turned out not to work since the current
sm_context framework doesn't support applying two conditions
simultaneously: it led to a transition from the old state to has_lb,
then a transition from the old state *again* to has_ub, thus leaving
the new state as has_ub, rather than the stop state.
Instead, this patch fixes things by special-casing it within
taint_state_machine::on_condition.
gcc/analyzer/ChangeLog:
PR analyzer/106284
* sm-taint.cc (taint_state_machine::on_condition): Handle range
checks optimized by build_range_check.
gcc/testsuite/ChangeLog:
PR analyzer/106284
* gcc.dg/analyzer/torture/taint-read-index-2.c: New test.
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bug analyzer/106284] False positives from -Wanalyzer-tainted-array-index with optimized conditionals
2022-07-13 20:08 [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals dmalcolm at gcc dot gnu.org
2022-07-14 21:09 ` [Bug analyzer/106284] " dmalcolm at gcc dot gnu.org
2022-07-15 15:30 ` cvs-commit at gcc dot gnu.org
@ 2022-07-15 15:41 ` dmalcolm at gcc dot gnu.org
2022-07-19 13:56 ` cvs-commit at gcc dot gnu.org
3 siblings, 0 replies; 5+ messages in thread
From: dmalcolm at gcc dot gnu.org @ 2022-07-15 15:41 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106284
David Malcolm <dmalcolm at gcc dot gnu.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Status|ASSIGNED |RESOLVED
Resolution|--- |FIXED
--- Comment #3 from David Malcolm <dmalcolm at gcc dot gnu.org> ---
Should be fixed on trunk for gcc 13 by the above patch.
^ permalink raw reply [flat|nested] 5+ messages in thread
* [Bug analyzer/106284] False positives from -Wanalyzer-tainted-array-index with optimized conditionals
2022-07-13 20:08 [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals dmalcolm at gcc dot gnu.org
` (2 preceding siblings ...)
2022-07-15 15:41 ` dmalcolm at gcc dot gnu.org
@ 2022-07-19 13:56 ` cvs-commit at gcc dot gnu.org
3 siblings, 0 replies; 5+ messages in thread
From: cvs-commit at gcc dot gnu.org @ 2022-07-19 13:56 UTC (permalink / raw)
To: gcc-bugs
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=106284
--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by David Malcolm <dmalcolm@gcc.gnu.org>:
https://gcc.gnu.org/g:434d521d118fc7e7759b2b42bdddfa70caec637b
commit r13-1747-g434d521d118fc7e7759b2b42bdddfa70caec637b
Author: David Malcolm <dmalcolm@redhat.com>
Date: Tue Jul 19 09:53:39 2022 -0400
analyzer: log out-edge description in exploded_graph::process_node
I found this logging tweak very helpful when working on
PR analyzer/106284.
gcc/analyzer/ChangeLog:
* engine.cc (exploded_graph::process_node): Show any description
of the out-edge when logging it for consideration.
Signed-off-by: David Malcolm <dmalcolm@redhat.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-07-19 13:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-13 20:08 [Bug analyzer/106284] New: False positives from -Wanalyzer-tainted-array-index with optimized conditionals dmalcolm at gcc dot gnu.org
2022-07-14 21:09 ` [Bug analyzer/106284] " dmalcolm at gcc dot gnu.org
2022-07-15 15:30 ` cvs-commit at gcc dot gnu.org
2022-07-15 15:41 ` dmalcolm at gcc dot gnu.org
2022-07-19 13:56 ` cvs-commit at gcc dot gnu.org
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 read-only IMAP folder(s) and NNTP newsgroup(s).