[PATCH] osmo-mgw[master]: mgcp: fix use-after-free and add callback for endpoint cleanup

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

dexter gerrit-no-reply at lists.osmocom.org
Mon Feb 5 10:50:12 UTC 2018


Hello Harald Welte, Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/6006

to look at the new patch set (#6).

mgcp: fix use-after-free and add callback for endpoint cleanup

Since we will support multiple different types of endpoints in the
future, all these endpoints will handle connections slightly different
and there will be possibly state that needs to be kept consistant
when a connection is deleted.

In mgcp_network.c where we implement the callback that is used to
create an rtp-bride-endpoint. In that callback we cache the pointer
of the connection we where we want to bride to (opposite connection).
When one of the connections is deleted using a DLCX operation, the
pointer is still there and the next incoming packet causes a use-
after-free segfault.

- introduce an endpoint specific callback function that is executed
  before removing the connection.

- implement the endpoint specific callback for rtp bridge endpoints,
  so that the use-after-free is prevented.

Change-Id: I921d9bbe58be1c3298e164a37f3c974880b3759f
---
M include/osmocom/mgcp/mgcp_endp.h
M include/osmocom/mgcp/mgcp_internal.h
M src/libosmo-mgcp/mgcp_conn.c
M src/libosmo-mgcp/mgcp_endp.c
M src/libosmo-mgcp/mgcp_network.c
5 files changed, 38 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-mgw refs/changes/06/6006/6

diff --git a/include/osmocom/mgcp/mgcp_endp.h b/include/osmocom/mgcp/mgcp_endp.h
index 0225879..a486dcd 100644
--- a/include/osmocom/mgcp/mgcp_endp.h
+++ b/include/osmocom/mgcp/mgcp_endp.h
@@ -33,6 +33,13 @@
 				     char *buf, unsigned int buf_size,
 				     struct mgcp_conn *conn);
 
+/* Callback type for endpoint specific cleanup actions. This function
+ * is automatically executed when a connection is freed (see mgcp_conn_free()
+ * in mgcp_conn.c). Depending on the type of the endpoint there may be endpoint
+ * specific things to take care of once a connection has been removed. */
+typedef void (*mgcp_cleanup_cp) (struct mgcp_endpoint *endp,
+				 struct mgcp_conn *conn);
+
 /*! MGCP endpoint properties */
 struct mgcp_endpoint_type {
 	/*!< maximum number of connections */
@@ -40,6 +47,9 @@
 
 	/*!< callback that defines how to dispatch incoming RTP data */
 	mgcp_dispatch_rtp_cb dispatch_rtp_cb;
+
+	/*!< callback that implements endpoint specific cleanup actions */
+	mgcp_cleanup_cp cleanup_cb;
 };
 
 /*! MGCP endpoint typeset */
diff --git a/include/osmocom/mgcp/mgcp_internal.h b/include/osmocom/mgcp/mgcp_internal.h
index 9c57e3f..e4009e8 100644
--- a/include/osmocom/mgcp/mgcp_internal.h
+++ b/include/osmocom/mgcp/mgcp_internal.h
@@ -265,6 +265,7 @@
 int mgcp_send_dummy(struct mgcp_endpoint *endp, struct mgcp_conn_rtp *conn);
 int mgcp_dispatch_rtp_bridge_cb(int proto, struct sockaddr_in *addr, char *buf,
 				unsigned int buf_size, struct mgcp_conn *conn);
+void mgcp_cleanup_rtp_bridge_cb(struct mgcp_endpoint *endp, struct mgcp_conn *conn);
 int mgcp_bind_net_rtp_port(struct mgcp_endpoint *endp, int rtp_port,
 			   struct mgcp_conn_rtp *conn);
 void mgcp_free_rtp_port(struct mgcp_rtp_end *end);
diff --git a/src/libosmo-mgcp/mgcp_conn.c b/src/libosmo-mgcp/mgcp_conn.c
index cc115a9..62cbdba 100644
--- a/src/libosmo-mgcp/mgcp_conn.c
+++ b/src/libosmo-mgcp/mgcp_conn.c
@@ -201,6 +201,12 @@
 	if (!conn)
 		return;
 
+	/* Run endpoint cleanup action. By this we inform the endpoint about
+	 * the removal of the connection and allow it to clean up its inner
+	 * state accordingly */
+	if (endp->type->cleanup_cb)
+		endp->type->cleanup_cb(endp, conn);
+
 	switch (conn->type) {
 	case MGCP_CONN_TYPE_RTP:
 		osmux_disable_conn(&conn->u.rtp);
diff --git a/src/libosmo-mgcp/mgcp_endp.c b/src/libosmo-mgcp/mgcp_endp.c
index 6d3a6d4..fa2dd28 100644
--- a/src/libosmo-mgcp/mgcp_endp.c
+++ b/src/libosmo-mgcp/mgcp_endp.c
@@ -28,7 +28,8 @@
 const struct mgcp_endpoint_typeset ep_typeset = {
 	/* Specify endpoint properties for RTP endpoint */
 	.rtp.max_conns = 2,
-	.rtp.dispatch_rtp_cb = mgcp_dispatch_rtp_bridge_cb
+	.rtp.dispatch_rtp_cb = mgcp_dispatch_rtp_bridge_cb,
+	.rtp.cleanup_cb = mgcp_cleanup_rtp_bridge_cb
 };
 
 /*! release endpoint, all open connections are closed.
diff --git a/src/libosmo-mgcp/mgcp_network.c b/src/libosmo-mgcp/mgcp_network.c
index ef6357b..6923b97 100644
--- a/src/libosmo-mgcp/mgcp_network.c
+++ b/src/libosmo-mgcp/mgcp_network.c
@@ -1043,6 +1043,25 @@
 
 }
 
+/*! cleanup an endpoint when a connection on an RTP bridge endpoint is removed.
+ *  \param[in] endp Endpoint on which the connection resides.
+ *  \param[in] conn Connection that is about to be removed (ignored).
+ *  \returns 0 on success, -1 on ERROR. */
+void mgcp_cleanup_rtp_bridge_cb(struct mgcp_endpoint *endp, struct mgcp_conn *conn)
+{
+	struct mgcp_conn *conn_cleanup;
+
+	/* In mgcp_dispatch_rtp_bridge_cb() we use conn->priv to cache the
+	 * pointer to the destination connection, so that we do not have
+	 * to go through the list every time an RTP packet arrives. To prevent
+	 * a use-after-free situation we invalidate this information for all
+	 * connections present when one connection is removed from the
+	 * endpoint. */
+	llist_for_each_entry(conn_cleanup, &endp->conns, entry) {
+		conn_cleanup->priv = NULL;
+	}
+}
+
 /* Handle incoming RTP data from NET */
 static int rtp_data_net(struct osmo_fd *fd, unsigned int what)
 {

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I921d9bbe58be1c3298e164a37f3c974880b3759f
Gerrit-PatchSet: 6
Gerrit-Project: osmo-mgw
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier at sysmocom.de>



More information about the gerrit-log mailing list