summaryrefslogtreecommitdiffstats
path: root/src/vppinfra
diff options
context:
space:
mode:
authorPaul Atkins <patkins@graphiant.com>2022-04-06 14:51:21 +0100
committerDamjan Marion <dmarion@me.com>2022-04-08 15:35:39 +0000
commitdfc43164078b481e39dc0a87e8e358cc6a56d14e (patch)
treea4367aef179208043d964d77d17b41dd469c5a5a /src/vppinfra
parent853530b528d57506236c6e2b5ffc02fc976cdc68 (diff)
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
Diffstat (limited to 'src/vppinfra')
-rw-r--r--src/vppinfra/interrupt.h4
-rw-r--r--src/vppinfra/test_interrupt.c79
2 files changed, 82 insertions, 1 deletions
diff --git a/src/vppinfra/interrupt.h b/src/vppinfra/interrupt.h
index 393574b076b..5c39b2183f8 100644
--- 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
index 00000000000..519805edc89
--- /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;
+}