@@ -848,6 +848,48 @@ taint_state_machine::on_condition (sm_context *sm_ctxt,
case LE_EXPR:
case LT_EXPR:
{
+ /* Detect where build_range_check has optimized
+ (c>=low) && (c<=high)
+ into
+ (c-low>=0) && (c-low<=high-low)
+ and thus into:
+ (unsigned)(c - low) <= (unsigned)(high-low). */
+ if (const binop_svalue *binop_sval
+ = lhs->dyn_cast_binop_svalue ())
+ {
+ const svalue *inner_lhs = binop_sval->get_arg0 ();
+ enum tree_code inner_op = binop_sval->get_op ();
+ const svalue *inner_rhs = binop_sval->get_arg1 ();
+ if (const svalue *before_cast = inner_lhs->maybe_undo_cast ())
+ inner_lhs = before_cast;
+ if (tree outer_rhs_cst = rhs->maybe_get_constant ())
+ if (tree inner_rhs_cst = inner_rhs->maybe_get_constant ())
+ if (inner_op == PLUS_EXPR
+ && TREE_CODE (inner_rhs_cst) == INTEGER_CST
+ && TREE_CODE (outer_rhs_cst) == INTEGER_CST
+ && TYPE_UNSIGNED (TREE_TYPE (inner_rhs_cst))
+ && TYPE_UNSIGNED (TREE_TYPE (outer_rhs_cst)))
+ {
+ /* We have
+ (unsigned)(INNER_LHS + CST_A) </<= UNSIGNED_CST_B
+ and thus an optimized test of INNER_LHS (before any
+ cast to unsigned) against a range.
+ Transition any of the tainted states to the stop state.
+ We have to special-case this here rather than in
+ region_model::on_condition since we can't apply
+ both conditions simultaneously (we'd have a transition
+ from the old state to has_lb, then a transition from
+ the old state *again* to has_ub). */
+ state_t old_state
+ = sm_ctxt->get_state (stmt, inner_lhs);
+ if (old_state == m_tainted
+ || old_state == m_has_lb
+ || old_state == m_has_ub)
+ sm_ctxt->set_next_state (stmt, inner_lhs, m_stop);
+ return;
+ }
+ }
+
sm_ctxt->on_transition (node, stmt, lhs, m_tainted,
m_has_ub);
sm_ctxt->on_transition (node, stmt, lhs, m_has_lb,
new file mode 100644
@@ -0,0 +1,56 @@
+// TODO: remove need for the taint option:
+/* { dg-additional-options "-fanalyzer-checker=taint" } */
+/* { dg-skip-if "" { *-*-* } { "-fno-fat-lto-objects" } { "" } } */
+
+#define LOWER_LIMIT 5
+#define UPPER_LIMIT 20
+
+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" } */
+}
+
+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" } */
+}
+
+static int
+called_by_test_4 (int iarg)
+{
+ if (iarg < LOWER_LIMIT || iarg > UPPER_LIMIT)
+ return 0;
+ return arr[iarg]; /* { dg-bogus "bounds checking" } */
+}
+
+int __attribute__((tainted_args))
+test_4 (unsigned uarg)
+{
+ return called_by_test_4 (uarg);
+}
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. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-1713-g0a8edfbd37d399. 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> --- gcc/analyzer/sm-taint.cc | 42 ++++++++++++++ .../analyzer/torture/taint-read-index-2.c | 56 +++++++++++++++++++ 2 files changed, 98 insertions(+) create mode 100644 gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c