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>

2 files changed, 98 insertions, 0 deletions

diff --git a/gcc/analyzer/sm-taint.cc b/gcc/analyzer/sm-taint.cc
index 4075cf6..2de9284 100644
--- a/gcc/analyzer/sm-taint.cc
+++ b/gcc/analyzer/sm-taint.cc
@@ -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,
diff --git a/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c b/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
new file mode 100644
index 0000000..6a4ebdb
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/analyzer/torture/taint-read-index-2.c
@@ -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);
+}