lynxis lazus has submitted this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/43510?usp=email )
Change subject: libvlr: auth_fsm: split off fsm creation and start of it ......................................................................
libvlr: auth_fsm: split off fsm creation and start of it
The auth fsm may destroy itself from a chain of fsm events by VLR_AUTH_E_START. Resulting in an use-after-free of the fsm instance.
Change-Id: Ibad920676d9d4a8b7de4a820196385738764ffe9 --- M src/libvlr/vlr_access_req_fsm.c M src/libvlr/vlr_auth_fsm.c M src/libvlr/vlr_auth_fsm.h M src/libvlr/vlr_lu_fsm.c 4 files changed, 20 insertions(+), 6 deletions(-)
Approvals: Jenkins Builder: Verified pespin: Looks good to me, approved
diff --git a/src/libvlr/vlr_access_req_fsm.c b/src/libvlr/vlr_access_req_fsm.c index 5d2b7f9..f68e5fc 100644 --- a/src/libvlr/vlr_access_req_fsm.c +++ b/src/libvlr/vlr_access_req_fsm.c @@ -363,12 +363,13 @@ if (is_auth_to_be_attempted(par)) { osmo_fsm_inst_state_chg(fi, PR_ARQ_S_WAIT_AUTH, 0, 0); - vsub->auth_fsm = auth_fsm_start(vsub, fi, + vsub->auth_fsm = auth_fsm_create(vsub, fi, PR_ARQ_E_AUTH_RES, PR_ARQ_E_AUTH_NO_INFO, PR_ARQ_E_AUTH_FAILURE, par->is_r99, par->is_utran); + auth_fsm_start(vsub->auth_fsm); } else { _proc_arq_vlr_node2(fi); } diff --git a/src/libvlr/vlr_auth_fsm.c b/src/libvlr/vlr_auth_fsm.c index 192e390..7345037 100644 --- a/src/libvlr/vlr_auth_fsm.c +++ b/src/libvlr/vlr_auth_fsm.c @@ -642,7 +642,7 @@ ***********************************************************************/
/* MSC->VLR: Start Procedure Authenticate_VLR (TS 23.012 Ch. 4.1.2.2) */ -struct osmo_fsm_inst *auth_fsm_start(struct vlr_subscr *vsub, +struct osmo_fsm_inst *auth_fsm_create(struct vlr_subscr *vsub, struct osmo_fsm_inst *parent, uint32_t parent_event_success, uint32_t parent_event_no_auth_info, @@ -676,11 +676,17 @@ fi->priv = afp; vsub->auth_fsm = fi;
- osmo_fsm_inst_dispatch(fi, VLR_AUTH_E_START, NULL); - return fi; }
+int auth_fsm_start(struct osmo_fsm_inst *fi) +{ + if (fi) + return osmo_fsm_inst_dispatch(fi, VLR_AUTH_E_START, NULL); + + return -ENOENT; +} + bool auth_ciph_sec_ctx_is_usable(const struct vlr_subscr *vsub, uint8_t key_seq) { int max_reuse_count = vsub->vlr->cfg.ciph_sec_ctx_max_reuse; diff --git a/src/libvlr/vlr_auth_fsm.h b/src/libvlr/vlr_auth_fsm.h index 7a45a1a..078960b 100644 --- a/src/libvlr/vlr_auth_fsm.h +++ b/src/libvlr/vlr_auth_fsm.h @@ -29,7 +29,7 @@ VLR_AUTH_E_MS_ID_IMSI, };
-struct osmo_fsm_inst *auth_fsm_start(struct vlr_subscr *vsub, +struct osmo_fsm_inst *auth_fsm_create(struct vlr_subscr *vsub, struct osmo_fsm_inst *parent, uint32_t parent_event_success, uint32_t parent_event_no_auth_info, @@ -37,6 +37,8 @@ bool is_r99, bool is_utran);
+int auth_fsm_start(struct osmo_fsm_inst *fi); + void vlr_auth_fsm_init(bool is_ps); void vlr_auth_fsm_set_log_subsys(int log_level); bool auth_try_reuse_tuple(struct vlr_subscr *vsub, uint8_t key_seq); diff --git a/src/libvlr/vlr_lu_fsm.c b/src/libvlr/vlr_lu_fsm.c index d3aaf3a..e97f19d 100644 --- a/src/libvlr/vlr_lu_fsm.c +++ b/src/libvlr/vlr_lu_fsm.c @@ -991,17 +991,22 @@
OSMO_ASSERT(vsub);
+ /* check if we need to do an authentication, if only ciphering, check if ciphering key is available */ if (try_auth(lfp)) { /* Authenticate_VLR */ osmo_fsm_inst_state_chg(fi, VLR_ULA_S_WAIT_AUTH, LU_TIMEOUT_LONG, 0); - vsub->auth_fsm = auth_fsm_start(lfp->vsub, + /* First only create and assign vsub->auth_fsm, because an auth_fsm_start() might run into a failure + * and the fsm will be free'ed. + */ + vsub->auth_fsm = auth_fsm_create(lfp->vsub, fi, VLR_ULA_E_AUTH_SUCCESS, VLR_ULA_E_AUTH_NO_INFO, VLR_ULA_E_AUTH_FAILURE, lfp->is_r99, lfp->is_utran); + auth_fsm_start(vsub->auth_fsm); } else { /* no need for authentication */ vlr_loc_upd_post_auth(fi);