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

2 files changed, 20 insertions, 70 deletions

diff --git a/src/libmsc/a_iface.c b/src/libmsc/a_iface.c
index e2fe3c6b9..b769b0aa4 100644
--- a/src/libmsc/a_iface.c
+++ b/src/libmsc/a_iface.c
@@ -534,7 +534,6 @@ static int sccp_sap_up(struct osmo_prim_hdr *oph, void *_scu)
 				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 @@ static int sccp_sap_up(struct osmo_prim_hdr *oph, void *_scu)
 		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 01cb71d85..0946a5d4b 100644
--- a/src/libmsc/a_iface_bssap.c
+++ b/src/libmsc/a_iface_bssap.c
@@ -121,8 +121,6 @@ static void bssmap_rx_reset(struct osmo_sccp_user *scu, const struct a_conn_info
 
 	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 @@ static void bssmap_rx_reset_ack(const struct osmo_sccp_user *scu, const struct a
 	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 @@ static void bssmap_rx_reset_ack(const struct osmo_sccp_user *scu, const struct a
 	/* 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 @@ static void bssmap_rcvmsg_udt(struct osmo_sccp_user *scu, const struct a_conn_in
 
 	if (msgb_l3len(msg) < 1) {
 		LOGP(DBSSAP, LOGL_NOTICE, "Error: No data received -- discarding message!\n");
-		msgb_free(msg);
 		return;
 	}
 
@@ -177,7 +171,6 @@ static void bssmap_rcvmsg_udt(struct osmo_sccp_user *scu, const struct a_conn_in
 	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 @@ void a_sccp_rx_udt(struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_
 
 	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 @@ void a_sccp_rx_udt(struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_
 	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 @@ static int bssmap_rx_clear_rqst(struct gsm_subscriber_connection *conn, struct m
 	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 @@ static int bssmap_rx_clear_rqst(struct gsm_subscriber_connection *conn, struct m
 
 	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 @@ static int bssmap_rx_clear_complete(struct osmo_sccp_user *scu,
 	/* 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 @@ static int bssmap_rx_l3_compl(struct osmo_sccp_user *scu, const struct a_conn_in
 	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 @@ static int bssmap_rx_l3_compl(struct osmo_sccp_user *scu, const struct a_conn_in
 	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 @@ static int bssmap_rx_l3_compl(struct osmo_sccp_user *scu, const struct a_conn_in
 
 	/* 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");
@@ -345,10 +329,6 @@ static int bssmap_rx_l3_compl(struct osmo_sccp_user *scu, const struct a_conn_in
 		LOGP(DMSC, LOGL_INFO, "User has been rejected by MSC (unknown error)\n");
 
 	return -EINVAL;
-
-fail:
-	msgb_free(msg);
-	return -EINVAL;
 }
 
 /* Endpoint to handle BSSMAP classmark update */
@@ -365,7 +345,7 @@ static int bssmap_rx_classmark_upd(struct gsm_subscriber_connection *conn, struc
 	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 @@ static int bssmap_rx_classmark_upd(struct gsm_subscriber_connection *conn, struc
 	/* 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 @@ static int bssmap_rx_ciph_compl(struct gsm_subscriber_connection *conn, struct m
 		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 @@ static int bssmap_rx_ciph_rej(struct gsm_subscriber_connection *conn, struct msg
 	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 @@ static int bssmap_rx_ciph_rej(struct gsm_subscriber_connection *conn, struct msg
 	/* 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 @@ static int bssmap_rx_ass_fail(struct gsm_subscriber_connection *conn, struct msg
 	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 @@ static int bssmap_rx_ass_fail(struct gsm_subscriber_connection *conn, struct msg
 	/* 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 @@ static int bssmap_rx_sapi_n_rej(struct gsm_subscriber_connection *conn, struct m
 	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 @@ static int bssmap_rx_ass_compl(struct gsm_subscriber_connection *conn, struct ms
 
 	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 @@ static int bssmap_rx_ass_compl(struct gsm_subscriber_connection *conn, struct ms
 					 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 @@ static int bssmap_rx_ass_compl(struct gsm_subscriber_connection *conn, struct ms
 		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 @@ static int rx_bssmap(struct osmo_sccp_user *scu, const struct a_conn_info *a_con
 
 	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 @@ static int rx_bssmap(struct osmo_sccp_user *scu, const struct a_conn_info *a_con
 	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 @@ static int rx_bssmap(struct osmo_sccp_user *scu, const struct a_conn_info *a_con
 		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 @@ static int rx_dtap(const struct osmo_sccp_user *scu, const struct a_conn_info *a
 
 	conn = subscr_conn_lookup_a(network, a_conn_info->conn_id);
 	if (!conn) {
-		msgb_free(msg);
 		return -EINVAL;
 	}
 
@@ -647,12 +598,11 @@ static int rx_dtap(const struct osmo_sccp_user *scu, const struct a_conn_info *a
 
 	/* 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 @@ int a_sccp_rx_dt(struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_in
 
 	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 @@ int a_sccp_rx_dt(struct osmo_sccp_user *scu, const struct a_conn_info *a_conn_in
 		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;
 	}