This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
fixeria gerrit-no-reply at lists.osmocom.orgfixeria has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/18907 ) Change subject: fix crashes due to OSMO_ASSERT(conn->lchan) ...................................................................... fix crashes due to OSMO_ASSERT(conn->lchan) Starting from ttcn3-bsc-test-sccplite build #777, it was noticed that osmo-bsc crashes with the following message: Assert failed conn->lchan include/osmocom/bsc/gsm_data.h:1376 The cause of this is a recently merged patch that calls conn_get_bts() during assignment_fsm rate counter dispatch: "Count assignment rates per BTS as well" commit b5ccf09fc4042c7fb1fdaaa6263961c40b32564e Change-Id I0009e51d4caf68e762138d98e2e23d49acc3cc1a The root cause being that the assignment_fsm attempts to count an Assignment event for a BTS after the lchan has already been released and disassociated from the conn. The assertion is found in conn_get_bts(), which is used in various places. In fact, each caller is a potential DoS risk -- though most are in code paths that are guaranteed to have an lchan and bts present, having an OSMO_ASSERT() on the relatively volatile presence of an lchan is not a good idea for osmo-bsc's stability and error resilience. - Change conn_get_bts() to return NULL in the lack of an lchan. - Adjust all callers of conn_get_bts() to gracefully handle a NULL return val. - Same for cgi_for_msc() and callers, closely related. Here is a backtrace: Program received signal SIGABRT pwndbg> bt 0x0000555555be6e52 in conn_get_bts (conn=0x622000057160) at include/osmocom/bsc/gsm_data.h:1376 0x0000555555c1edc8 in assignment_fsm_timer_cb (fi=0x612000060220) at assignment_fsm.c:758 0x00007ffff72b1104 in fsm_tmr_cb (data=0x612000060220) at libosmocore/src/fsm.c:325 0x00007ffff72ab062 in osmo_timers_update () at libosmocore/src/timer.c:257 0x00007ffff72ab5d2 in _osmo_select_main (polling=0) at libosmocore/src/select.c:260 0x00007ffff72abd2f in osmo_select_main_ctx (polling=<optimized out>) at libosmocore/src/select.c:291 0x0000555555e1b81b in main (argc=3, argv=0x7fffffffe1b8) at osmo_bsc_main.c:953 0x00007ffff6752002 in __libc_start_main () from /usr/lib/libc.so.6 0x0000555555b61bbe in _start () In the case of the assignment_fsm counter, we now miss a chance to increase a BTS counter for a failed Assignment, but this is a separate problem. The main point of this patch is that osmo-bsc must not crash. Related: OS#4620, OS#4619 Patch-by: fixeria Tweaked-by: neels Fixes: I0009e51d4caf68e762138d98e2e23d49acc3cc1a Change-Id: Id681dfb0ad654bdb4b71805d1ad4f39a8bf6bbd1 --- M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/assignment_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/gsm_08_08.c M src/osmo-bsc/gsm_data.c M src/osmo-bsc/osmo_bsc_bssap.c M src/osmo-bsc/osmo_bsc_filter.c M src/osmo-bsc/osmo_bsc_msc.c M src/osmo-bsc/osmo_bsc_sigtran.c 9 files changed, 72 insertions(+), 25 deletions(-) Approvals: daniel: Looks good to me, approved pespin: Looks good to me, but someone else must approve Jenkins Builder: Verified diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h index 8c1c545..ce18d1b 100644 --- a/include/osmocom/bsc/gsm_data.h +++ b/include/osmocom/bsc/gsm_data.h @@ -1373,7 +1373,8 @@ static inline struct gsm_bts *conn_get_bts(struct gsm_subscriber_connection *conn) { - OSMO_ASSERT(conn->lchan); + if (!conn || !conn->lchan) + return NULL; return conn->lchan->ts->trx->bts; } diff --git a/src/osmo-bsc/assignment_fsm.c b/src/osmo-bsc/assignment_fsm.c index 94dd359..cd5abc8 100644 --- a/src/osmo-bsc/assignment_fsm.c +++ b/src/osmo-bsc/assignment_fsm.c @@ -80,7 +80,8 @@ bsc_ctr_description[BSC_##counter].name, \ bsc_ctr_description[BSC_##counter].description); \ rate_ctr_inc(&conn->network->bsc_ctrs->ctr[BSC_##counter]); \ - rate_ctr_inc(&bts->bts_ctrs->ctr[BTS_##counter]); \ + if (bts) \ + rate_ctr_inc(&bts->bts_ctrs->ctr[BTS_##counter]); \ } while(0) #define assignment_count_result(counter) do { \ diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c index 9896bff..2941e5b 100644 --- a/src/osmo-bsc/bsc_vty.c +++ b/src/osmo-bsc/bsc_vty.c @@ -1776,7 +1776,10 @@ /* Find the connection/lchan that we want to handover */ llist_for_each_entry(conn, &net->subscr_conns, entry) { - if (conn_get_bts(conn)->nr == bts_nr && + struct gsm_bts *bts = conn_get_bts(conn); + if (!bts) + continue; + if (bts->nr == bts_nr && conn->lchan->ts->trx->nr == trx_nr && conn->lchan->ts->nr == ts_nr && conn->lchan->nr == ss_nr) { vty_out(vty, "starting %s for lchan %s...%s", action, conn->lchan->name, VTY_NEWLINE); diff --git a/src/osmo-bsc/gsm_08_08.c b/src/osmo-bsc/gsm_08_08.c index 3703c76..e288506 100644 --- a/src/osmo-bsc/gsm_08_08.c +++ b/src/osmo-bsc/gsm_08_08.c @@ -222,8 +222,9 @@ /* Has the subscriber been paged from a connected MSC? */ if (pdisc == GSM48_PDISC_RR && mtype == GSM48_MT_RR_PAG_RESP) { subscr = bsc_subscr_find_by_mi(conn->network->bsc_subscribers, &mi); - if (subscr) { - msc_target = paging_get_msc(conn_get_bts(conn), subscr); + struct gsm_bts *bts = conn_get_bts(conn); + if (bts && subscr) { + msc_target = paging_get_msc(bts, subscr); bsc_subscr_put(subscr); if (is_msc_usable(msc_target, is_emerg)) { LOG_COMPL_L3(pdisc, mtype, LOGL_DEBUG, "%s matches earlier Paging from msc %d\n", @@ -356,6 +357,13 @@ { struct osmo_mobile_identity mi; struct bsc_subscr *subscr = NULL; + struct gsm_bts *bts = conn_get_bts(conn); + + if (!bts) { + /* should never happen */ + LOGP(DRSL, LOGL_ERROR, "Paging Response without lchan\n"); + return -1; + } if (osmo_mobile_identity_decode_from_l3(&mi, msg, false)) { LOGP(DRSL, LOGL_ERROR, "Unable to extract Mobile Identity from Paging Response\n"); @@ -370,8 +378,7 @@ return -1; } - paging_request_stop(&conn->network->bts_list, conn_get_bts(conn), subscr, conn, - msg); + paging_request_stop(&conn->network->bts_list, bts, subscr, conn, msg); bsc_subscr_put(subscr); return 0; } @@ -385,6 +392,11 @@ int8_t rc8; struct gsm_bts *bts = conn_get_bts(conn); + if (!bts) { + /* should never happen */ + LOGP(DRSL, LOGL_ERROR, "LU Request without lchan\n"); + return; + } if (msgb_l3len(msg) < sizeof(*gh) + sizeof(*lu)) { LOGP(DMSC, LOGL_ERROR, "LU too small to look at: %u\n", msgb_l3len(msg)); @@ -415,6 +427,12 @@ int8_t rc8; struct gsm_bts *bts = conn_get_bts(conn); + if (!bts) { + /* should never happen */ + LOGP(DRSL, LOGL_ERROR, "CM Service Request without lchan\n"); + return; + } + if (msgb_l3len(msg) < sizeof(*gh) + sizeof(*serv_req)) { LOGP(DMSC, LOGL_ERROR, "CM Serv Req too small to look at: %u\n", msgb_l3len(msg)); return; @@ -462,6 +480,15 @@ struct msgb *resp; struct gsm0808_speech_codec_list scl; int rc = -2; + struct gsm_bts *bts = conn_get_bts(conn); + struct osmo_cell_global_id *cgi = cgi_for_msc(conn->sccp.msc, bts); + + if (!bts || !cgi) { + /* should never happen */ + LOGP(DMSC, LOGL_ERROR, "Compl L3 without lchan\n"); + rc = -1; + goto early_fail; + } log_set_context(LOG_CTX_BSC_SUBSCR, conn->bsub); @@ -482,9 +509,9 @@ bsc_scan_bts_msg(conn, msg); if (gscon_is_aoip(conn)) { - gen_bss_supported_codec_list(&scl, msc, conn_get_bts(conn)); + gen_bss_supported_codec_list(&scl, msc, bts); if (scl.len > 0) - resp = gsm0808_create_layer3_2(msg, cgi_for_msc(conn->sccp.msc, conn_get_bts(conn)), &scl); + resp = gsm0808_create_layer3_2(msg, cgi, &scl); else { /* Note: 3GPP TS 48.008 3.2.1.32, COMPLETE LAYER 3 INFORMATION clearly states that * Codec List (BSS Supported) shall be included, if the radio access network @@ -492,10 +519,10 @@ * current configuration does not support any voice codecs, in those cases the * network does not support an IP based user plane interface, and therefore the * Codec List (BSS Supported) IE can be left out in those situations. */ - resp = gsm0808_create_layer3_2(msg, cgi_for_msc(conn->sccp.msc, conn_get_bts(conn)), NULL); + resp = gsm0808_create_layer3_2(msg, cgi, NULL); } } else - resp = gsm0808_create_layer3_2(msg, cgi_for_msc(conn->sccp.msc, conn_get_bts(conn)), NULL); + resp = gsm0808_create_layer3_2(msg, cgi, NULL); if (resp) rc = osmo_fsm_inst_dispatch(conn->fi, GSCON_EV_A_CONN_REQ, resp); @@ -537,6 +564,12 @@ struct msgb *resp; struct gsm_bts *bts = conn_get_bts(conn); + if (!bts) { + /* should never happen */ + LOGP(DMSC, LOGL_ERROR, "Classmark Update without lchan\n"); + return; + } + rc8 = osmo_gsm48_rfpowercap2powerclass(bts->band, cm2_parsed->pwr_lev); if (rc8 < 0) { LOGP(DMSC, LOGL_NOTICE, diff --git a/src/osmo-bsc/gsm_data.c b/src/osmo-bsc/gsm_data.c index 9bf6f82..2098f13 100644 --- a/src/osmo-bsc/gsm_data.c +++ b/src/osmo-bsc/gsm_data.c @@ -1721,7 +1721,7 @@ /* If there's an associated lchan, attempt to update its max power to be on track with band maximum values */ - if (conn->lchan) + if (bts && conn->lchan) lchan_update_ms_power_ctrl_level(conn->lchan, bts->ms_max_power); } diff --git a/src/osmo-bsc/osmo_bsc_bssap.c b/src/osmo-bsc/osmo_bsc_bssap.c index 6b225e4..2deb7f4 100644 --- a/src/osmo-bsc/osmo_bsc_bssap.c +++ b/src/osmo-bsc/osmo_bsc_bssap.c @@ -478,7 +478,6 @@ struct msgb *msg, unsigned int payload_length) { uint16_t len; - struct gsm_network *network = NULL; const uint8_t *data; struct tlv_parsed tp; struct msgb *resp; @@ -522,7 +521,6 @@ goto reject; } - network = conn_get_bts(conn)->network; data = TLVP_VAL(&tp, GSM0808_IE_ENCRYPTION_INFORMATION); enc_bits_msc = data[0]; enc_key = &data[1]; @@ -540,10 +538,10 @@ /* The bit-mask of permitted ciphers from the MSC (sent in ASSIGNMENT COMMAND) is intersected * with the vty-configured mask a the BSC. Finally, the best (highest) possible cipher is * chosen. */ - chosen_cipher = select_best_cipher(enc_bits_msc, network->a5_encryption_mask); + chosen_cipher = select_best_cipher(enc_bits_msc, bsc_gsmnet->a5_encryption_mask); if (chosen_cipher < 0) { LOGP(DMSC, LOGL_ERROR, "Reject: no overlapping A5 ciphers between BSC (0x%02x) " - "and MSC (0x%02x)\n", network->a5_encryption_mask, enc_bits_msc); + "and MSC (0x%02x)\n", bsc_gsmnet->a5_encryption_mask, enc_bits_msc); reject_cause = GSM0808_CAUSE_CIPHERING_ALGORITHM_NOT_SUPPORTED; goto reject; } @@ -663,17 +661,23 @@ { int rc, i, nc = 0; struct bsc_msc_data *msc; + struct gsm_bts *bts = conn_get_bts(conn); + + if (!bts) { + LOGP(DMSC, LOGL_ERROR, "No lchan, cannot select codecs\n"); + return -EINVAL; + } msc = conn->sccp.msc; switch (ct->ch_rate_type) { case GSM0808_SPEECH_FULL_BM: - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_FR); nc += (rc == 0); break; case GSM0808_SPEECH_HALF_LM: - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_HR); nc += (rc == 0); break; @@ -681,19 +685,19 @@ case GSM0808_SPEECH_PERM_NO_CHANGE: case GSM0808_SPEECH_FULL_PREF_NO_CHANGE: case GSM0808_SPEECH_FULL_PREF: - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_FR); nc += (rc == 0); - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_HR); nc += (rc == 0); break; case GSM0808_SPEECH_HALF_PREF_NO_CHANGE: case GSM0808_SPEECH_HALF_PREF: - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_HR); nc += (rc == 0); - rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, conn_get_bts(conn), + rc = match_codec_pref(&req->ch_mode_rate[nc], ct, &conn->codec_list, msc, bts, RATE_PREF_FR); nc += (rc == 0); break; diff --git a/src/osmo-bsc/osmo_bsc_filter.c b/src/osmo-bsc/osmo_bsc_filter.c index 98b5148..08c3a50 100644 --- a/src/osmo-bsc/osmo_bsc_filter.c +++ b/src/osmo-bsc/osmo_bsc_filter.c @@ -132,11 +132,12 @@ msc = conn->sccp.msc; if (mtype == GSM48_MT_MM_LOC_UPD_ACCEPT) { - if (has_core_identity(msc)) { + struct gsm_bts *bts = conn_get_bts(conn); + if (bts && has_core_identity(msc)) { if (msgb_l3len(msg) >= sizeof(*gh) + sizeof(*lai)) { /* overwrite LAI in the message */ lai = (struct gsm48_loc_area_id *) &gh->data[0]; - gsm48_generate_lai2(lai, bts_lai(conn_get_bts(conn))); + gsm48_generate_lai2(lai, bts_lai(bts)); } } return 0; diff --git a/src/osmo-bsc/osmo_bsc_msc.c b/src/osmo-bsc/osmo_bsc_msc.c index 9b00ffc..f969146 100644 --- a/src/osmo-bsc/osmo_bsc_msc.c +++ b/src/osmo-bsc/osmo_bsc_msc.c @@ -266,6 +266,10 @@ struct osmo_cell_global_id *cgi_for_msc(struct bsc_msc_data *msc, struct gsm_bts *bts) { static struct osmo_cell_global_id cgi; + + if (!bts) + return NULL; + cgi.lai.plmn = msc->network->plmn; if (msc->core_plmn.mcc != GSM_MCC_MNC_INVALID) cgi.lai.plmn.mcc = msc->core_plmn.mcc; diff --git a/src/osmo-bsc/osmo_bsc_sigtran.c b/src/osmo-bsc/osmo_bsc_sigtran.c index f5489e4..43ffae0 100644 --- a/src/osmo-bsc/osmo_bsc_sigtran.c +++ b/src/osmo-bsc/osmo_bsc_sigtran.c @@ -311,7 +311,7 @@ return BSC_CON_REJECT_NO_LINK; } - if (!bsc_grace_allow_new_connection(bts->network, bts)) { + if (bts && !bsc_grace_allow_new_connection(bts->network, bts)) { LOGP(DMSC, LOGL_NOTICE, "BSC in grace period. No new connections.\n"); return BSC_CON_REJECT_RF_GRACE; } -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/18907 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: Id681dfb0ad654bdb4b71805d1ad4f39a8bf6bbd1 Gerrit-Change-Number: 18907 Gerrit-PatchSet: 6 Gerrit-Owner: fixeria <vyanitskiy at sysmocom.de> Gerrit-Assignee: neels <nhofmeyr at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel <dwillmann at sysmocom.de> Gerrit-Reviewer: dexter <pmaier at sysmocom.de> Gerrit-Reviewer: fixeria <axilirator at gmail.com> Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de> Gerrit-Reviewer: laforge <laforge at osmocom.org> Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-CC: neels <nhofmeyr at sysmocom.de> Gerrit-MessageType: merged -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200623/8ac38726/attachment.htm>