[PATCH] osmo-msc[master]: a_iface: Fix heap-use-after-free by cleaning up msgb ownership

Harald Welte gerrit-no-reply at lists.osmocom.org
Fri Feb 9 20:42:07 UTC 2018


Review at  https://gerrit.osmocom.org/6355

a_iface: Fix heap-use-after-free by cleaning up msgb ownership

When we receive a msgb-wrapped primitive from the SCCP provider (stack),
it transfers msgb ownership to us (the SCCP user).  The existing code
passed the msgb ownership down into all the various downstream
functions, which each then had to take care of msgb free'ing.

Not all of the paths did eventually free the msgb.  And at least one
path used data from the primitive *after* the free

Let's restructure this in a way that no msgb ownership is transferred
down the call chain.  Instead, there's one common msgb_free() in
sccp_sap_up().  We can do this as nobody is queueing or otherwise
keeping the msgb.

Change-Id: Ie65616ccb55ec58a0224bbe3c8e004e6029ef3e6
SUMMARY: AddressSanitizer: heap-use-after-free /home/laforge/projects/git/osmo-msc/src/libmsc/a_iface.c:538 in sccp_sap_up
---
M src/libmsc/a_iface.c
M src/libmsc/a_iface_bssap.c
M src/libmsc/msc_vty.c
3 files changed, 25 insertions(+), 70 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/55/6355/1

diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index e2fe3c6..b769b0a 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -534,7 +534,6 @@
 				rc = a_sccp_rx_dt(scu, &a_conn_info, oph->msg);
 			} else
 				LOGP(DBSSAP, LOGL_DEBUG, "N-CONNECT.ind(%u)\n", scu_prim->u.connect.conn_id);
-
 			record_bsc_con(scu, a_conn_info.bsc, scu_prim->u.connect.conn_id);
 		}
 		break;
@@ -586,6 +585,9 @@
 		break;
 	}
 
+	/* We didn't transfer msgb ownership to any downstream functions so we rely on
+	 * this single/central location to free() the msgb wrapping the primitive */
+	msgb_free(oph->msg);
 	return rc;
 }
 
diff --git a/src/libmsc/a_iface_bssap.c b/src/libmsc/a_iface_bssap.c
index 01cb71d..0946a5d 100644
--- a/src/libmsc/a_iface_bssap.c
+++ b/src/libmsc/a_iface_bssap.c
@@ -121,8 +121,6 @@
 
 	if (!a_conn_info->bsc->reset)
 		a_start_reset(a_conn_info->bsc, true);
-
-	msgb_free(msg);
 }
 
 /* Endpoint to handle BSSMAP reset acknowlegement */
@@ -139,7 +137,7 @@
 	if (a_conn_info->bsc->reset == NULL) {
 		LOGP(DBSSAP, LOGL_ERROR, "Received RESET ACK from an unknown BSC %s, ignoring...\n",
 		     osmo_sccp_addr_name(ss7, &a_conn_info->bsc->bsc_addr));
-		goto fail;
+		return;
 	}
 
 	LOGP(DBSSAP, LOGL_NOTICE, "Received RESET ACK from BSC %s\n",
@@ -148,9 +146,6 @@
 	/* Confirm that we managed to get the reset ack message
 	 * towards the connection reset logic */
 	a_reset_ack_confirm(a_conn_info->bsc->reset);
-
-fail:
-	msgb_free(msg);
 }
 
 /* Handle UNITDATA BSSMAP messages */
@@ -161,7 +156,6 @@
 
 	if (msgb_l3len(msg) < 1) {
 		LOGP(DBSSAP, LOGL_NOTICE, "Error: No data received -- discarding message!\n");
-		msgb_free(msg);
 		return;
 	}
 
@@ -177,7 +171,6 @@
 	default:
 		LOGP(DBSSAP, LOGL_NOTICE, "Unimplemented message format: %s -- message discarded!\n",
 		     gsm0808_bssmap_name(msg->l3h[0]));
-		msgb_free(msg);
 	}
 }
 
@@ -196,14 +189,12 @@
 
 	if (msgb_l2len(msg) < sizeof(*bs)) {
 		LOGP(DBSSAP, LOGL_ERROR, "Error: Header is too short -- discarding message!\n");
-		msgb_free(msg);
 		return;
 	}
 
 	bs = (struct bssmap_header *)msgb_l2(msg);
 	if (bs->length < msgb_l2len(msg) - sizeof(*bs)) {
 		LOGP(DBSSAP, LOGL_ERROR, "Error: Message is too short -- discarding message!\n");
-		msgb_free(msg);
 		return;
 	}
 
@@ -215,7 +206,6 @@
 	default:
 		LOGP(DBSSAP, LOGL_ERROR,
 		     "Error: Unimplemented message type: %s -- message discarded!\n", gsm0808_bssmap_name(bs->type));
-		msgb_free(msg);
 	}
 }
 
@@ -236,7 +226,7 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_CAUSE)) {
 		LOGP(DBSSAP, LOGL_ERROR, "Cause code is missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	cause = TLVP_VAL(&tp, GSM0808_IE_CAUSE)[0];
 
@@ -246,11 +236,7 @@
 
 	msc_clear_request(conn, cause);
 
-	msgb_free(msg);
 	return rc;
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle BSSMAP clear complete */
@@ -266,7 +252,6 @@
 	/* Remove the record from the list with active connections. */
 	a_delete_bsc_con(a_conn_info->conn_id);
 
-	msgb_free(msg);
 	return rc;
 }
 
@@ -294,11 +279,11 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_CELL_IDENTIFIER)) {
 		LOGP(DBSSAP, LOGL_ERROR, "Mandatory CELL IDENTIFIER not present -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_LAYER_3_INFORMATION)) {
 		LOGP(DBSSAP, LOGL_ERROR, "Mandatory LAYER 3 INFORMATION not present -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	/* Parse Cell ID element */
@@ -310,18 +295,18 @@
 	if (sizeof(lai_ci) != data_length) {
 		LOGP(DBSSAP, LOGL_ERROR,
 		     "Unable to parse element CELL IDENTIFIER (wrong field length) -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	memcpy(&lai_ci, data, sizeof(lai_ci));
 	if (lai_ci.ident != CELL_IDENT_WHOLE_GLOBAL) {
 		LOGP(DBSSAP, LOGL_ERROR,
 		     "Unable to parse element CELL IDENTIFIER (wrong cell identification discriminator) -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	if (gsm48_decode_lai(&lai_ci.lai, &mcc, &mnc, &lac) != 0) {
 		LOGP(DBSSAP, LOGL_ERROR,
 		     "Unable to parse element CELL IDENTIFIER (lai decoding failed) -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	/* Parse Layer 3 Information element */
@@ -334,7 +319,6 @@
 
 	/* Handover location update to the MSC code */
 	rc = msc_compl_l3(conn, msg, 0);
-	msgb_free(msg);
 
 	if (rc == MSC_CONN_ACCEPT) {
 		LOGP(DMSC, LOGL_INFO, "User has been accepted by MSC.\n");
@@ -344,10 +328,6 @@
 	else
 		LOGP(DMSC, LOGL_INFO, "User has been rejected by MSC (unknown error)\n");
 
-	return -EINVAL;
-
-fail:
-	msgb_free(msg);
 	return -EINVAL;
 }
 
@@ -365,7 +345,7 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_CLASSMARK_INFORMATION_T2)) {
 		LOGPCONN(conn, LOGL_ERROR, "Mandatory Classmark Information Type 2 not present -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	cm2 = TLVP_VAL(&tp, GSM0808_IE_CLASSMARK_INFORMATION_T2);
@@ -379,12 +359,7 @@
 	/* Inform MSC about the classmark change */
 	msc_classmark_chg(conn, cm2, cm2_len, cm3, cm3_len);
 
-	msgb_free(msg);
 	return 0;
-
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle BSSMAP cipher mode complete */
@@ -412,13 +387,11 @@
 		msg->l3h = (uint8_t*)TLVP_VAL(&tp, GSM0808_IE_LAYER_3_MESSAGE_CONTENTS);
 		msg->tail = msg->l3h + TLVP_LEN(&tp, GSM0808_IE_LAYER_3_MESSAGE_CONTENTS);
 	} else {
-		msgb_free(msg);
 		msg = NULL;
 	}
 
 	/* Hand over cipher mode complete message to the MSC */
 	msc_cipher_mode_compl(conn, msg, alg_id);
-	msgb_free(msg);
 
 	return 0;
 }
@@ -434,7 +407,7 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, BSS_MAP_MSG_CIPHER_MODE_REJECT)) {
 		LOGPCONN(conn, LOGL_ERROR, "Cause code is missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	cause = TLVP_VAL(&tp, BSS_MAP_MSG_CIPHER_MODE_REJECT)[0];
@@ -443,11 +416,7 @@
 	/* FIXME: Can we do something meaningful here? e.g. report to the
 	 * msc code somehow that the cipher mode command has failed. */
 
-	msgb_free(msg);
 	return 0;
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle BSSMAP assignment failure */
@@ -463,7 +432,7 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_CAUSE)) {
 		LOGPCONN(conn, LOGL_ERROR, "Cause code is missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	cause = TLVP_VAL(&tp, GSM0808_IE_CAUSE)[0];
 
@@ -481,12 +450,7 @@
 	/* Inform the MSC about the assignment failure event */
 	msc_assign_fail(conn, cause, rr_cause_ptr);
 
-	msgb_free(msg);
 	return 0;
-
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle sapi "n" reject */
@@ -503,25 +467,20 @@
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_CAUSE)) {
 		LOGPCONN(conn, LOGL_ERROR, "Cause code is missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	tlv_parse(&tp, gsm0808_att_tlvdef(), msg->l3h + 1, msgb_l3len(msg) - 1, 0, 0);
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_DLCI)) {
 		LOGPCONN(conn, LOGL_ERROR, "DLCI is missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 	dlci = TLVP_VAL(&tp, GSM0808_IE_DLCI)[0];
 
 	/* Inform the MSC about the sapi "n" reject event */
 	msc_sapi_n_reject(conn, dlci);
 
-	msgb_free(msg);
 	return 0;
-
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle assignment complete */
@@ -542,7 +501,7 @@
 
 	if (!TLVP_PRESENT(&tp, GSM0808_IE_AOIP_TRASP_ADDR)) {
 		LOGPCONN(conn, LOGL_ERROR, "AoIP transport identifier missing -- discarding message!\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	/* Decode AoIP transport address element */
@@ -550,7 +509,7 @@
 					 TLVP_LEN(&tp, GSM0808_IE_AOIP_TRASP_ADDR));
 	if (rc < 0) {
 		LOGPCONN(conn, LOGL_ERROR, "Unable to decode aoip transport address.\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	/* use address / port supplied with the AoIP
@@ -560,18 +519,14 @@
 		msc_mgcp_ass_complete(conn, osmo_ntohs(rtp_addr_in->sin_port), inet_ntoa(rtp_addr_in->sin_addr));
 	} else {
 		LOGPCONN(conn, LOGL_ERROR, "Unsopported addressing scheme. (supports only IPV4)\n");
-		goto fail;
+		return -EINVAL;
 	}
 
 	/* FIXME: Seems to be related to authentication or,
 	   encryption. Is this really in the right place? */
 	msc_rx_sec_mode_compl(conn);
 
-	msgb_free(msg);
 	return 0;
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Handle incoming connection oriented BSSMAP messages */
@@ -581,7 +536,6 @@
 
 	if (msgb_l3len(msg) < 1) {
 		LOGP(DBSSAP, LOGL_NOTICE, "Error: No data received -- discarding message!\n");
-		msgb_free(msg);
 		return -1;
 	}
 
@@ -598,7 +552,6 @@
 	conn = subscr_conn_lookup_a(a_conn_info->network, a_conn_info->conn_id);
 	if (!conn) {
 		LOGP(DBSSAP, LOGL_ERROR, "Couldn't find subscr_conn for conn_id=%d\n", a_conn_info->conn_id);
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
@@ -621,14 +574,13 @@
 		return bssmap_rx_ass_compl(conn, msg);
 	default:
 		LOGPCONN(conn, LOGL_ERROR, "Unimplemented msg type: %s\n", gsm0808_bssmap_name(msg->l3h[0]));
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
 	return -EINVAL;
 }
 
-/* Endpoint to handle regular BSSAP DTAP messages */
+/* Endpoint to handle regular BSSAP DTAP messages. No ownership of 'msg' is passed on! */
 static int rx_dtap(const struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_info, struct msgb *msg)
 {
 	struct gsm_network *network = a_conn_info->network;
@@ -636,7 +588,6 @@
 
 	conn = subscr_conn_lookup_a(network, a_conn_info->conn_id);
 	if (!conn) {
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
@@ -647,12 +598,11 @@
 
 	/* Forward dtap payload into the msc */
 	msc_dtap(conn, conn->a.conn_id, msg);
-	msgb_free(msg);
 
 	return 0;
 }
 
-/* Handle incoming connection oriented messages */
+/* Handle incoming connection oriented messages. No ownership of 'msg' is passed on! */
 int a_sccp_rx_dt(struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_info, struct msgb *msg)
 {
 	OSMO_ASSERT(scu);
@@ -661,7 +611,6 @@
 
 	if (msgb_l2len(msg) < sizeof(struct bssmap_header)) {
 		LOGP(DBSSAP, LOGL_NOTICE, "The header is too short -- discarding message!\n");
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
@@ -673,7 +622,6 @@
 		return rx_dtap(scu, a_conn_info, msg);
 	default:
 		LOGP(DBSSAP, LOGL_ERROR, "Unimplemented BSSAP msg type: %s\n", gsm0808_bssap_name(msg->l2h[0]));
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index faf17ec..c72a0b7 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -149,6 +149,11 @@
 	if (gsmnet->paging_response_timer != MSC_PAGING_RESPONSE_TIMER_DEFAULT)
 		vty_out(vty, " paging response-timer %u%s", gsmnet->paging_response_timer, VTY_NEWLINE);
 
+	if (gsmnet->emergency.route_to_msisdn) {
+		vty_out(vty, " emergency-call route-to-msisdn %s%s",
+			gsmnet->emergency.route_to_msisdn, VTY_NEWLINE);
+	}
+
 	mgcp_client_config_write(vty, " ");
 #ifdef BUILD_IU
 	ranap_iu_vty_config_write(vty, " ");

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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie65616ccb55ec58a0224bbe3c8e004e6029ef3e6
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Harald Welte <laforge at gnumonks.org>


More information about the gerrit-log mailing list