From 65c56c83ce4e58178b5ad90a8f325692c9904381 Mon Sep 17 00:00:00 2001 From: Ole Troan Date: Wed, 21 Oct 2020 11:55:28 +0200 Subject: 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 Change-Id: I94f124ec71d98218c4eda5d124ac5594743d93d6 --- src/vpp-api/client/stat_client.c | 24 ++++++++++++++++++------ src/vpp-api/client/stat_client.h | 8 ++++++-- src/vpp/stats/stat_segment.c | 3 ++- 3 files changed, 26 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..f8473efd47f 100644 --- a/src/vpp-api/client/stat_client.h +++ b/src/vpp-api/client/stat_client.h @@ -101,8 +101,12 @@ _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 (p > (void *) sm->shared_header && + ((p + sizeof (p)) < ((void *) 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 b060969f04f..4244acb1dd0 100644 --- a/src/vpp/stats/stat_segment.c +++ b/src/vpp/stats/stat_segment.c @@ -281,7 +281,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; } -- cgit 1.2.3-korg