Hello Jenkins Builder,

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

    https://gerrit.osmocom.org/6355

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

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
2 files changed, 20 insertions(+), 70 deletions(-)


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

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;
        }
 

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie65616ccb55ec58a0224bbe3c8e004e6029ef3e6
Gerrit-PatchSet: 2
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Harald Welte <lafo...@gnumonks.org>
Gerrit-Reviewer: Jenkins Builder

Reply via email to