From 1a5bcd5c3b3c84dbd1bf99fe08eaab51370fbef9 Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Sat, 18 Nov 2017 22:19:55 +0100 Subject: sub_pres_vlr_fsm_start: fix heap use after free When sub_pres_vlr_fsm_start() is called, it dispatches an event which may in some cases already cause tear down and free of the parent FSM instance, after which storing the returned instance pointer in that parent's metadata will use freed memory. Instead, pass the target pointer to remember the instance at to sub_pres_vlr_fsm_start() and assign the pointer *before* firing the event. Explain so in a new comment. I haven't checked whether that pointer is actually used at all -- this is the easiest way to fix the use-after-free without getting sucked into semantic questions. Change-Id: Ibdc0b64cd12ba3e2b9737e3517d8484e67abcf04 --- include/osmocom/msc/vlr.h | 7 ++++--- src/libvlr/vlr_access_req_fsm.c | 3 +-- src/libvlr/vlr_lu_fsm.c | 24 +++++++++++++++--------- 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h index d5306fa81..9e6b12c33 100644 --- a/include/osmocom/msc/vlr.h +++ b/include/osmocom/msc/vlr.h @@ -274,9 +274,10 @@ int vlr_start(const char *gsup_unit_name, struct vlr_instance *vlr, /* internal use only */ -struct osmo_fsm_inst *sub_pres_vlr_fsm_start(struct osmo_fsm_inst *parent, - struct vlr_subscr *vsub, - uint32_t term_event); +void sub_pres_vlr_fsm_start(struct osmo_fsm_inst **fsm, + struct osmo_fsm_inst *parent, + struct vlr_subscr *vsub, + uint32_t term_event); struct osmo_fsm_inst * upd_hlr_vlr_proc_start(struct osmo_fsm_inst *parent, struct vlr_subscr *vsub, diff --git a/src/libvlr/vlr_access_req_fsm.c b/src/libvlr/vlr_access_req_fsm.c index 41620b90d..e90d8de9f 100644 --- a/src/libvlr/vlr_access_req_fsm.c +++ b/src/libvlr/vlr_access_req_fsm.c @@ -241,8 +241,7 @@ static void _proc_arq_vlr_node2_post_vlr(struct osmo_fsm_inst *fi) if (vsub->ms_not_reachable_flag) { /* Start Subscriber_Present_VLR */ osmo_fsm_inst_state_chg(fi, PR_ARQ_S_WAIT_SUB_PRES, 0, 0); - par->sub_pres_vlr_fsm = sub_pres_vlr_fsm_start(fi, vsub, - PR_ARQ_E_PRES_RES); + sub_pres_vlr_fsm_start(&par->sub_pres_vlr_fsm, fi, vsub, PR_ARQ_E_PRES_RES); return; } _proc_arq_vlr_post_pres(fi); diff --git a/src/libvlr/vlr_lu_fsm.c b/src/libvlr/vlr_lu_fsm.c index 37fe2352c..a3a68ed98 100644 --- a/src/libvlr/vlr_lu_fsm.c +++ b/src/libvlr/vlr_lu_fsm.c @@ -252,21 +252,29 @@ static struct osmo_fsm sub_pres_vlr_fsm = { .event_names = sub_pres_vlr_event_names, }; -struct osmo_fsm_inst *sub_pres_vlr_fsm_start(struct osmo_fsm_inst *parent, - struct vlr_subscr *vsub, - uint32_t term_event) +/* Note that the start event is dispatched right away, so in case the FSM immediately concludes from that + * event, the created FSM struct may no longer be valid as it already deallocated again, and it may + * furthermore already have invoked the parent FSM instance's deallocation as well. Hence, instead of + * returning, store the created FSM instance address in *fi_p before dispatching the event. It is thus + * possible to store the instance's pointer in a parent FSM instance without running danger of using + * already freed memory. */ +void sub_pres_vlr_fsm_start(struct osmo_fsm_inst **fi_p, + struct osmo_fsm_inst *parent, + struct vlr_subscr *vsub, + uint32_t term_event) { struct osmo_fsm_inst *fi; + OSMO_ASSERT(fi_p); + fi = osmo_fsm_inst_alloc_child(&sub_pres_vlr_fsm, parent, term_event); + *fi_p = fi; if (!fi) - return NULL; + return; fi->priv = vsub; osmo_fsm_inst_dispatch(fi, SUB_PRES_VLR_E_START, NULL); - - return fi; } /*********************************************************************** @@ -384,9 +392,7 @@ static void lu_compl_vlr_init(struct osmo_fsm_inst *fi, uint32_t event, osmo_fsm_inst_state_chg(fi, LU_COMPL_VLR_S_WAIT_SUB_PRES, LU_TIMEOUT_LONG, 0); - lcvp->sub_pres_vlr_fsm = sub_pres_vlr_fsm_start(fi, vsub, - LU_COMPL_VLR_E_SUB_PRES_COMPL); - + sub_pres_vlr_fsm_start(&lcvp->sub_pres_vlr_fsm, fi, vsub, LU_COMPL_VLR_E_SUB_PRES_COMPL); } static void lu_compl_vlr_new_tmsi(struct osmo_fsm_inst *fi) -- cgit v1.2.3