Change in openbsc[master]: bsc_nat: Fix crash (double-free) in forward_sccp_to_msc

[Pau Espin Pedrol]

Pau Espin Pedrol has uploaded this change for review. ( https://gerrit.osmocom.org/13837


Change subject: bsc_nat: Fix crash (double-free) in forward_sccp_to_msc
......................................................................

bsc_nat: Fix crash (double-free) in forward_sccp_to_msc

In bsc_nat_parse(), parsed is allocated this way:
"""parsed = talloc_zero(msg, struct bsc_nat_parsed);"""
So parsed is a child of msg, and so it's freed when msg is freed.

Since libosmocore c7f52c4c84d6a8898048738c4db9266289c40b45,
osmo_wqueue_enqueue() correctly detects queue full and returns an error,
and then queue_for_msc() calls msgb_free(). Code in osmo-bsc-nat was
probably written before that change in behavior, so that's why probably
the bug was not hit before.

Related: SYS#4548
Change-Id: I209d3e2d809a67915ec43c874e68f7f746a565f0
---
M openbsc/src/osmo-bsc_nat/bsc_nat.c
1 file changed, 11 insertions(+), 6 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/37/13837/1

diff --git a/openbsc/src/osmo-bsc_nat/bsc_nat.c b/openbsc/src/osmo-bsc_nat/bsc_nat.c
index 670b0be..fb34aa7 100644
--- a/openbsc/src/osmo-bsc_nat/bsc_nat.c
+++ b/openbsc/src/osmo-bsc_nat/bsc_nat.c
@@ -108,19 +108,23 @@
 	return NULL;
 }
 
-static void queue_for_msc(struct bsc_msc_connection *con, struct msgb *msg)
+static int queue_for_msc(struct bsc_msc_connection *con, struct msgb *msg)
 {
+	int rc;
 	if (!con) {
 		LOGP(DLINP, LOGL_ERROR, "No MSC Connection assigned. Check your code.\n");
 		msgb_free(msg);
-		return;
+		return -EINVAL;
 	}
 
-
-	if (osmo_wqueue_enqueue(&con->write_queue, msg) != 0) {
+	rc = osmo_wqueue_enqueue(&con->write_queue, msg);
+	if (rc != 0) {
 		LOGP(DLINP, LOGL_ERROR, "Failed to enqueue the write.\n");
 		msgb_free(msg);
+		return rc;
 	}
+
+	return 0;
 }
 
 static void send_reset_ack(struct bsc_connection *bsc)
@@ -1277,9 +1281,10 @@
 	}
 
 	/* send the non-filtered but maybe modified msg */
-	queue_for_msc(con_msc, msg);
-	if (parsed)
+	if (queue_for_msc(con_msc, msg) == 0 && parsed)
 		talloc_free(parsed);
+	/* else: enqueue error, msg has been freed, and so child parsed is freed too */
+
 	return 0;
 
 exit:

-- 
To view, visit https://gerrit.osmocom.org/13837
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: openbsc
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I209d3e2d809a67915ec43c874e68f7f746a565f0
Gerrit-Change-Number: 13837
Gerrit-PatchSet: 1
Gerrit-Owner: Pau Espin Pedrol <pespin at sysmocom.de>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190501/7224709f/attachment.html>