[MERGED] osmo-msc[master]: sub_pres_vlr_fsm_start: fix heap use after free

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: sub_pres_vlr_fsm_start: fix heap use after free
......................................................................


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
---
M include/osmocom/msc/vlr.h
M src/libvlr/vlr_access_req_fsm.c
M src/libvlr/vlr_lu_fsm.c
3 files changed, 20 insertions(+), 14 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified
  Holger Freyther: Looks good to me, but someone else must approve



diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index d5306fa..9e6b12c 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -274,9 +274,10 @@
 
 /* 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 41620b9..e90d8de 100644
--- a/src/libvlr/vlr_access_req_fsm.c
+++ b/src/libvlr/vlr_access_req_fsm.c
@@ -241,8 +241,7 @@
 	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 37fe235..a3a68ed 100644
--- a/src/libvlr/vlr_lu_fsm.c
+++ b/src/libvlr/vlr_lu_fsm.c
@@ -252,21 +252,29 @@
 	.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 @@
 	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)

-- 
To view, visit https://gerrit.osmocom.org/4923
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ibdc0b64cd12ba3e2b9737e3517d8484e67abcf04
Gerrit-PatchSet: 2
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>