From df5d219f3964a3236d9ae844b423e8113552fde7 Mon Sep 17 00:00:00 2001 From: Philipp Maier Date: Wed, 24 Jan 2018 11:39:32 +0100 Subject: [PATCH] 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 --- include/osmocom/mgcp/mgcp_endp.h | 10 ++++++++++ include/osmocom/mgcp/mgcp_internal.h | 1 + src/libosmo-mgcp/mgcp_conn.c | 6 ++++++ src/libosmo-mgcp/mgcp_endp.c | 3 ++- src/libosmo-mgcp/mgcp_network.c | 19 +++++++++++++++++++ 5 files changed, 38 insertions(+), 1 deletion(-) diff --git a/include/osmocom/mgcp/mgcp_endp.h b/include/osmocom/mgcp/mgcp_endp.h index 022587979..a486dcd72 100644 --- a/include/osmocom/mgcp/mgcp_endp.h +++ b/include/osmocom/mgcp/mgcp_endp.h @@ -33,6 +33,13 @@ typedef int (*mgcp_dispatch_rtp_cb) (int proto, struct sockaddr_in *addr, 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 @@ struct mgcp_endpoint_type { /*!< 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 9c57e3fc5..e4009e8c2 100644 --- a/include/osmocom/mgcp/mgcp_internal.h +++ b/include/osmocom/mgcp/mgcp_internal.h @@ -265,6 +265,7 @@ int mgcp_send(struct mgcp_endpoint *endp, int is_rtp, struct sockaddr_in *addr, 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 cc115a905..62cbdbaa4 100644 --- a/src/libosmo-mgcp/mgcp_conn.c +++ b/src/libosmo-mgcp/mgcp_conn.c @@ -201,6 +201,12 @@ void mgcp_conn_free(struct mgcp_endpoint *endp, const char *id) 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 6d3a6d463..fa2dd281f 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 ef6357b1c..6923b97f7 100644 --- a/src/libosmo-mgcp/mgcp_network.c +++ b/src/libosmo-mgcp/mgcp_network.c @@ -1043,6 +1043,25 @@ int mgcp_dispatch_rtp_bridge_cb(int proto, struct sockaddr_in *addr, char *buf, } +/*! 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) {