From: Donald Sharp Date: Wed, 26 Feb 2025 17:34:05 +0000 (-0500) Subject: mgmtd: Prevent use after free X-Git-Tag: docker/10.2.2~3^2 X-Git-Url: https://git.puffer.fish/?a=commitdiff_plain;h=b4f3760f8630dbc16b717ee881a4e5b3a6910a01;p=matthieu%2Ffrr.git mgmtd: Prevent use after free ci is picking up this use after free on occasion: ERROR: AddressSanitizer: attempting to call malloc_usable_size() for pointer which is not owned: 0x6030001d94a0 0 0x7fab994b7f04 in __interceptor_malloc_usable_size ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:119 1 0x7fab994264f6 in __sanitizer::BufferedStackTrace::Unwind(unsigned long, unsigned long, void*, bool, unsigned int) ../../../../src/libsanitizer/sanitizer_common/sanitizer_stacktrace.h:131 2 0x7fab994264f6 in __asan::asan_malloc_usable_size(void const*, unsigned long, unsigned long) ../../../../src/libsanitizer/asan/asan_allocator.cpp:1058 3 0x7fab99039bcf in mt_count_free lib/memory.c:78 4 0x7fab99039bcf in qfree lib/memory.c:130 5 0x7fab98ff971a in hash_clean lib/hash.c:290 6 0x56110cdb0e7f in mgmt_txn_hash_destroy mgmtd/mgmt_txn.c:1881 7 0x56110cdb0e7f in mgmt_txn_destroy mgmtd/mgmt_txn.c:2013 8 0x56110cd8e5de in mgmt_terminate mgmtd/mgmt.c:91 9 0x56110cd8e003 in sigint mgmtd/mgmt_main.c:90 10 0x7fab990bf4b0 in frr_sigevent_process lib/sigevent.c:117 11 0x7fab990ea7a1 in event_fetch lib/event.c:1740 12 0x7fab9901a24e in frr_run lib/libfrr.c:1245 13 0x56110cd8e21f in main mgmtd/mgmt_main.c:290 14 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 15 0x7fab98af9304 in __libc_start_main_impl ../csu/libc-start.c:360 16 0x56110cd8dd30 in _start (/usr/lib/frr/mgmtd+0x3ad30) 0x6030001d94a0 is located 0 bytes inside of 24-byte region [0x6030001d94a0,0x6030001d94b8) freed by thread T0 here: 0 0x7fab994b76a8 in __interceptor_free ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:52 1 0x7fab99039bf0 in qfree lib/memory.c:131 2 0x7fab98ff93e1 in hash_release lib/hash.c:227 3 0x56110cdaabdc in mgmt_txn_unlock mgmtd/mgmt_txn.c:1931 4 0x56110cdab049 in mgmt_txn_delete mgmtd/mgmt_txn.c:1841 5 0x56110cdab0ce in mgmt_txn_hash_free mgmtd/mgmt_txn.c:1864 6 0x7fab98ff970b in hash_clean lib/hash.c:288 7 0x56110cdb0e7f in mgmt_txn_hash_destroy mgmtd/mgmt_txn.c:1881 8 0x56110cdb0e7f in mgmt_txn_destroy mgmtd/mgmt_txn.c:2013 9 0x56110cd8e5de in mgmt_terminate mgmtd/mgmt.c:91 10 0x56110cd8e003 in sigint mgmtd/mgmt_main.c:90 11 0x7fab990bf4b0 in frr_sigevent_process lib/sigevent.c:117 12 0x7fab990ea7a1 in event_fetch lib/event.c:1740 13 0x7fab9901a24e in frr_run lib/libfrr.c:1245 14 0x56110cd8e21f in main mgmtd/mgmt_main.c:290 15 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 previously allocated by thread T0 here: 0 0x7fab994b83b7 in __interceptor_calloc ../../../../src/libsanitizer/asan/asan_malloc_linux.cpp:77 1 0x7fab990392fd in qcalloc lib/memory.c:106 2 0x7fab98ff8b4f in hash_get lib/hash.c:156 3 0x56110cdb13ae in mgmt_txn_create_new mgmtd/mgmt_txn.c:1825 4 0x56110cdb3b4d in mgmt_txn_notify_be_adapter_conn mgmtd/mgmt_txn.c:2212 5 0x56110cd91178 in mgmt_be_adapter_conn_init mgmtd/mgmt_be_adapter.c:842 6 0x7fab990ec6de in event_call lib/event.c:2019 7 0x7fab9901a243 in frr_run lib/libfrr.c:1246 8 0x56110cd8e21f in main mgmtd/mgmt_main.c:290 9 0x7fab98af9249 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58 The only time that mgmt_txn_hash_free is called is in hash_clean. There are other places that mgmt_txn_unlock/delete are called and hash_release should be called. Let's just notice when mgmtd is being called from the hash_clean and not call hash_release (since we know it is being released already) Signed-off-by: Donald Sharp (cherry picked from commit 62f35c7bdb2a6364dd03ab120e7bb685dd317c24) --- diff --git a/mgmtd/mgmt_txn.c b/mgmtd/mgmt_txn.c index ccfdd7539f..5cfa665166 100644 --- a/mgmtd/mgmt_txn.c +++ b/mgmtd/mgmt_txn.c @@ -22,7 +22,7 @@ #define __log_err(fmt, ...) zlog_err("%s: ERROR: " fmt, __func__, ##__VA_ARGS__) #define MGMTD_TXN_LOCK(txn) mgmt_txn_lock(txn, __FILE__, __LINE__) -#define MGMTD_TXN_UNLOCK(txn) mgmt_txn_unlock(txn, __FILE__, __LINE__) +#define MGMTD_TXN_UNLOCK(txn, in_hash_free) mgmt_txn_unlock(txn, in_hash_free, __FILE__, __LINE__) enum mgmt_txn_event { MGMTD_TXN_PROC_SETCFG = 1, @@ -294,7 +294,7 @@ static inline const char *mgmt_txn_commit_phase_str(struct mgmt_txn_ctx *txn) } static void mgmt_txn_lock(struct mgmt_txn_ctx *txn, const char *file, int line); -static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file, +static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, bool in_hash_free, const char *file, int line); static int mgmt_txn_send_be_txn_delete(struct mgmt_txn_ctx *txn, struct mgmt_be_client_adapter *adapter); @@ -356,7 +356,7 @@ static void mgmt_txn_cfg_batch_free(struct mgmt_txn_be_cfg_batch **batch) } } - MGMTD_TXN_UNLOCK(&(*batch)->txn); + MGMTD_TXN_UNLOCK(&(*batch)->txn, false); XFREE(MTYPE_MGMTD_TXN_CFG_BATCH, *batch); *batch = NULL; @@ -551,7 +551,7 @@ static void mgmt_txn_req_free(struct mgmt_txn_req **txn_req) (*txn_req)->req_id, mgmt_txn_reqs_count(req_list)); } - MGMTD_TXN_UNLOCK(&(*txn_req)->txn); + MGMTD_TXN_UNLOCK(&(*txn_req)->txn, false); XFREE(MTYPE_MGMTD_TXN_REQ, (*txn_req)); *txn_req = NULL; } @@ -1835,9 +1835,9 @@ static struct mgmt_txn_ctx *mgmt_txn_create_new(uint64_t session_id, return txn; } -static void mgmt_txn_delete(struct mgmt_txn_ctx **txn) +static void mgmt_txn_delete(struct mgmt_txn_ctx **txn, bool in_hash_free) { - MGMTD_TXN_UNLOCK(txn); + MGMTD_TXN_UNLOCK(txn, in_hash_free); } static unsigned int mgmt_txn_hash_key(const void *data) @@ -1860,7 +1860,7 @@ static void mgmt_txn_hash_free(void *data) { struct mgmt_txn_ctx *txn = data; - mgmt_txn_delete(&txn); + mgmt_txn_delete(&txn, true); } static void mgmt_txn_hash_init(void) @@ -1910,8 +1910,7 @@ static void mgmt_txn_lock(struct mgmt_txn_ctx *txn, const char *file, int line) mgmt_txn_type2str(txn->type), txn->txn_id, txn->refcount); } -static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file, - int line) +static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, bool in_hash_free, const char *file, int line) { assert(*txn && (*txn)->refcount); @@ -1927,7 +1926,9 @@ static void mgmt_txn_unlock(struct mgmt_txn_ctx **txn, const char *file, EVENT_OFF((*txn)->proc_comm_cfg); EVENT_OFF((*txn)->comm_cfg_timeout); EVENT_OFF((*txn)->get_tree_timeout); - hash_release(mgmt_txn_mm->txn_hash, *txn); + if (!in_hash_free) + hash_release(mgmt_txn_mm->txn_hash, *txn); + mgmt_txns_del(&mgmt_txn_mm->txn_list, *txn); __dbg("Deleted %s txn-id: %" PRIu64 " session-id: %" PRIu64, @@ -1944,7 +1945,7 @@ static void mgmt_txn_cleanup_txn(struct mgmt_txn_ctx **txn) { /* TODO: Any other cleanup applicable */ - mgmt_txn_delete(txn); + mgmt_txn_delete(txn, false); } static void mgmt_txn_cleanup_all_txns(void) @@ -2036,7 +2037,7 @@ void mgmt_destroy_txn(uint64_t *txn_id) if (!txn) return; - mgmt_txn_delete(&txn); + mgmt_txn_delete(&txn, false); *txn_id = MGMTD_TXN_ID_NONE; }