[PATCH] osmo-msc[master]: a_iface: Fix heap-use-after-free by cleaning up msgb ownership
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,
[PATCH] osmo-msc[master]: a_iface: Fix heap-use-after-free by cleaning up msgb ownership
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_l3