From cae176e10a370f3e6829d172209f51866f235891 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Tue, 28 Jan 2025 16:37:52 +0100 Subject: [PATCH] lib: fix use after free in `clear event cpu` Freeing any item here means freeing someone's `event->hist`, leaving a dangling pointer there. Which will immediately be written to because we're executing in a CLI function under the `vty_read` event, whose `event->hist` is then updated. Deallocating `event->hist` anywhere other than shutting down the whole event loop is a bad idea to begin with, just zero out the stats instead. Fixes: FRRouting/frr#16419 Signed-off-by: David Lamparter --- lib/event.c | 22 +++++++++++----------- lib/frrevent.h | 8 ++++++++ 2 files changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/event.c b/lib/event.c index d95b3021a7..6081ba4727 100644 --- a/lib/event.c +++ b/lib/event.c @@ -111,6 +111,11 @@ static struct cpu_event_history *cpu_records_get(struct event_loop *loop, return res; } +static void cpu_records_clear(struct cpu_event_history *p) +{ + memset(p->_clear_begin, 0, p->_clear_end - p->_clear_begin); +} + static void cpu_records_free(struct cpu_event_history **p) { XFREE(MTYPE_EVENT_STATS, *p); @@ -250,20 +255,15 @@ static void cpu_record_clear(uint8_t filter) for (ALL_LIST_ELEMENTS_RO(masters, ln, m)) { frr_with_mutex (&m->mtx) { struct cpu_event_history *item; - struct cpu_records_head old[1]; - cpu_records_init(old); - cpu_records_swap_all(old, m->cpu_records); - - while ((item = cpu_records_pop(old))) { + /* it isn't possible to free the memory here + * because some of these will be in use (e.g. + * the one we're currently running in!) + */ + frr_each (cpu_records, m->cpu_records, item) { if (item->types & filter) - cpu_records_free(&item); - else - cpu_records_add(m->cpu_records, - item); + cpu_records_clear(item); } - - cpu_records_fini(old); } } } diff --git a/lib/frrevent.h b/lib/frrevent.h index 44776b29a7..c35b39a147 100644 --- a/lib/frrevent.h +++ b/lib/frrevent.h @@ -139,6 +139,10 @@ struct cpu_event_history { struct cpu_records_item item; void (*func)(struct event *e); + + /* fields between the pair of these two are nulled on "clear event cpu" */ + char _clear_begin[0]; + atomic_size_t total_cpu_warn; atomic_size_t total_wall_warn; atomic_size_t total_starv_warn; @@ -149,6 +153,10 @@ struct cpu_event_history { } real; struct time_stats cpu; atomic_uint_fast32_t types; + + /* end of cleared region */ + char _clear_end[0]; + const char *funcname; }; -- 2.39.5