stats: missing dimension in stat_set_simple_counter

A simple counter is a two dimensional array by threads and
counter index. 28017 introduced an error missing the first
dimension.

If a vector is updated at the same time as a client reads,
an invalid pointer my result. This will be caught by the
optimistic locking after copying out the data, but if
following a pointer outside of the stat segment then
the stat client would crash. Add suitable boundary checks
for access to stat memory segment.

Fixes: 7d29e320fb2855a1ddb7a6af09078b8ed636de01
Type: fix
Signed-off-by: Ole Troan <ot@cisco.com>
Change-Id: I94f124ec71d98218c4eda5d124ac5594743d93d6
(cherry picked from commit 65c56c83ce4e58178b5ad90a8f325692c9904381)
Signed-off-by: Elias Rudberg <elias.rudberg@bahnhof.net>

3 files changed, 27 insertions, 9 deletions

diff --git a/src/vpp-api/client/stat_client.c b/src/vpp-api/client/stat_client.c
index 56ff387d343..018cce34228 100644
--- a/src/vpp-api/client/stat_client.c
+++ b/src/vpp-api/client/stat_client.c
@@ -192,6 +192,16 @@ stat_segment_heartbeat (void)
   return stat_segment_heartbeat_r (sm);
 }
 
+#define stat_vec_dup(S,V)                             \
+  ({                                                  \
+  __typeof__ ((V)[0]) * _v(v) = 0;                    \
+  if (V && ((void *)V > (void *)S->shared_header) &&  \
+      (((void*)V + vec_bytes(V)) <                    \
+       ((void *)S->shared_header + S->memory_size)))  \
+    _v(v) = vec_dup(V);                               \
+   _v(v);                                             \
+})
+
 static stat_segment_data_t
 copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
 {
@@ -213,21 +223,21 @@ copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
 
     case STAT_DIR_TYPE_COUNTER_VECTOR_SIMPLE:
       simple_c = stat_segment_adjust (sm, ep->data);
-      result.simple_counter_vec = vec_dup (simple_c);
+      result.simple_counter_vec = stat_vec_dup (sm, simple_c);
       for (i = 0; i < vec_len (simple_c); i++)
 	{
 	  counter_t *cb = stat_segment_adjust (sm, simple_c[i]);
-	  result.simple_counter_vec[i] = vec_dup (cb);
+	  result.simple_counter_vec[i] = stat_vec_dup (sm, cb);
 	}
       break;
 
     case STAT_DIR_TYPE_COUNTER_VECTOR_COMBINED:
       combined_c = stat_segment_adjust (sm, ep->data);
-      result.combined_counter_vec = vec_dup (combined_c);
+      result.combined_counter_vec = stat_vec_dup (sm, combined_c);
       for (i = 0; i < vec_len (combined_c); i++)
 	{
 	  vlib_counter_t *cb = stat_segment_adjust (sm, combined_c[i]);
-	  result.combined_counter_vec[i] = vec_dup (cb);
+	  result.combined_counter_vec[i] = stat_vec_dup (sm, cb);
 	}
       break;
 
@@ -246,11 +256,11 @@ copy_data (stat_segment_directory_entry_t * ep, stat_client_main_t * sm)
     case STAT_DIR_TYPE_NAME_VECTOR:
       {
 	uint8_t **name_vector = stat_segment_adjust (sm, ep->data);
-	result.name_vector = vec_dup (name_vector);
+	result.name_vector = stat_vec_dup (sm, name_vector);
 	for (i = 0; i < vec_len (name_vector); i++)
 	  {
 	    u8 *name = stat_segment_adjust (sm, name_vector[i]);
-	    result.name_vector[i] = vec_dup (name);
+	    result.name_vector[i] = stat_vec_dup (sm, name);
 	  }
       }
       break;
@@ -290,6 +300,8 @@ stat_segment_data_free (stat_segment_data_t * res)
 	case STAT_DIR_TYPE_ERROR_INDEX:
 	  vec_free (res[i].error_vector);
 	  break;
+	case STAT_DIR_TYPE_SCALAR_INDEX:
+	  break;
 	default:
 	  assert (0);
 	}
diff --git a/src/vpp-api/client/stat_client.h b/src/vpp-api/client/stat_client.h
index c5fa5597758..b2457ad6e48 100644
--- a/src/vpp-api/client/stat_client.h
+++ b/src/vpp-api/client/stat_client.h
@@ -101,8 +101,13 @@ _time_now_nsec (void)
 static inline void *
 stat_segment_adjust (stat_client_main_t * sm, void *data)
 {
-  return (void *) ((char *) sm->shared_header +
-		   ((char *) data - (char *) sm->shared_header->base));
+  void *p = (void *) ((char *) sm->shared_header +
+		      ((char *) data - (char *) sm->shared_header->base));
+  if ((char *) p > (char *) sm->shared_header &&
+      (((char *) p + sizeof (p)) <
+       ((char *) sm->shared_header + sm->memory_size)))
+    return p;
+  return 0;
 }
 
 /*
diff --git a/src/vpp/stats/stat_segment.c b/src/vpp/stats/stat_segment.c
index 8255b16c010..788d2fddb2c 100644
--- a/src/vpp/stats/stat_segment.c
+++ b/src/vpp/stats/stat_segment.c
@@ -282,7 +282,8 @@ stat_set_simple_counter (stat_segment_directory_entry_t * ep,
 			 u32 thread_index, u32 index, u64 value)
 {
   ASSERT (ep->data);
-  counter_t *cb = ep->data;
+  counter_t **counters = ep->data;
+  counter_t *cb = counters[thread_index];
   cb[index] = value;
 }