vppinfra: clib_interrupt_get_next reading unallocated memory

The clib interrupt structure has a couple of fields at the start of
the cacheline, and then in the next cacheline it has a bitmap, which
is then followed by an atomic bitmap.  The size of the bitmaps is
based on the number of interrupts, and when the memory is allocated
the number of interrupts needed is used to size the overall block of
memory. The interrupts typically map to pool entries, so if we want
to store 512 entries then we store them in indices 0..511. This
would then take 8 6 4bit words, so each bitmap would be this size
when the struct is allocated.

It is possible to walk over the end of the allocated data with certain
sizes, one of which is 512. The reason this happens with 512 is that
the check to see when to exit the loop is returning when offset is
greater than the value needed to fit all the values.  In this case
512 >> 6 = 8. If there had only been 511 entries then the size would
have been 511 >> 6 = 7, and so it would have fitted in the space.

Therefore modify the check to also check that we are not looking into
the memory beyond what we have allocated in the case where the
number of interrupt is one of the boundary values like 512.

Also add a similar check first time round the loop as it is
possible we could have ate same problem there too.

Add a new test file to verify the new code works. The old version
of the code made this test fail when run with the address
sanitizer. Without the sanitiser it tended to pass because the
following memory was typically set to 0 even though it was
uninitialised.

Type: fix
Signed-off-by: Paul Atkins <patkins@graphiant.com>
Change-Id: I2ec4afae43d296a5c30299bd7694c072ca76b9a4

diff --git a/src/vppinfra/interrupt.h b/src/vppinfra/interrupt.h
index 393574b..5c39b21 100644 (file)
--- a/src/vppinfra/interrupt.h
+++ b/src/vppinfra/interrupt.h
@@ -110,6 +110,8 @@ clib_interrupt_get_next (void *in, int last)
   ASSERT (last >= -1 && last < h->n_int);
 
   off = (last + 1) >> log2_uword_bits;
+  if (off > h->n_int >> log2_uword_bits || off >= h->n_uword_alloc)
+    return -1;
 
   last -= off << log2_uword_bits;
   bmp[off] |= __atomic_exchange_n (abm + off, 0, __ATOMIC_SEQ_CST);
@@ -121,7 +123,7 @@ next:
 
   off++;
 
-  if (off > h->n_int >> log2_uword_bits)
+  if (off > h->n_int >> log2_uword_bits || off >= h->n_uword_alloc)
     return -1;
 
   bmp[off] |= __atomic_exchange_n (abm + off, 0, __ATOMIC_SEQ_CST);

diff --git a/src/vppinfra/test_interrupt.c b/src/vppinfra/test_interrupt.c
new file mode 100644 (file)
index 0000000..519805e
--- /dev/null
+++ b/src/vppinfra/test_interrupt.c
@@ -0,0 +1,79 @@
+/*
+ * Copyright (c) 2021 Graphiant, Inc.
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <vppinfra/clib.h>
+#include <vppinfra/format.h>
+#include <vppinfra/error.h>
+#include <vppinfra/random.h>
+#include <vppinfra/time.h>
+#include <vppinfra/interrupt.h>
+
+#define MAX_INTS 2048
+
+int debug = 0;
+
+#define debug(format, args...)                                                \
+  if (debug)                                                                  \
+    {                                                                         \
+      fformat (stdout, format, ##args);                                       \
+    }
+
+void
+set_and_check_bits (void *interrupts, int num_ints)
+{
+
+  int step;
+
+  for (step = 1; step < num_ints; step++)
+    {
+      int int_num = -1;
+      int expected = 0;
+
+      debug ("  Step of %d\n", step);
+      for (int i = 0; i < num_ints; i += step)
+       {
+         debug ("    Setting %d\n", i);
+         clib_interrupt_set (interrupts, i);
+       }
+
+      while ((int_num = clib_interrupt_get_next (interrupts, int_num)) != -1)
+       {
+         debug ("    Got %d, expecting %d\n", int_num, expected);
+         ASSERT (int_num == expected);
+         expected += step;
+         clib_interrupt_clear (interrupts, int_num);
+       }
+    }
+}
+
+int
+main (int argc, char *argv[])
+{
+  clib_mem_init (0, 3ULL << 30);
+
+  debug = (argc > 1);
+
+  void *interrupts = NULL;
+
+  for (int num_ints = 0; num_ints < MAX_INTS; num_ints++)
+    {
+      clib_interrupt_resize (&interrupts, num_ints);
+      debug ("Size now %d\n", num_ints);
+
+      set_and_check_bits (interrupts, num_ints);
+    }
+
+  return 0;
+}