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/.
neels gerrit-no-reply at lists.osmocom.orgneels has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/25182 ) Change subject: fix: handle NULL return of as_dl_tbf() and as_ul_tbf() ...................................................................... fix: handle NULL return of as_dl_tbf() and as_ul_tbf() Go through all callers of as_dl_tbf() and as_ul_tbf(), and make sure they can handle the possible NULL return value. OS#5205 reports a NULL deref crash of osmo-pcu at pdch.cpp:525. The immediate cause is that as_dl_tbf() may well return NULL, which this caller does not handle and instead dereferences immediately. This is a code path that apparently assumes that a DL-TBF should always be present. The higher level cause for the NULL DL-TBF has not been identified. Related: OS#5205 SYS#5561 Change-Id: I8ce21be6836549b47a606c00b793d6f005964c5c --- M src/bts.cpp M src/gprs_ms.c M src/gprs_rlcmac_sched.cpp M src/gprs_rlcmac_ts_alloc.cpp M src/pdch.cpp M src/tbf.cpp 6 files changed, 55 insertions(+), 35 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/82/25182/1 diff --git a/src/bts.cpp b/src/bts.cpp index ee6b915..80e6e83 100644 --- a/src/bts.cpp +++ b/src/bts.cpp @@ -1157,14 +1157,13 @@ { struct gprs_rlcmac_pdch *pdch = &bts->trx[trx_no].pdch[ts]; struct pdch_ulc_node *poll = pdch_ulc_get_node(pdch->ulc, fn); - struct gprs_rlcmac_ul_tbf *tbf; + struct gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(poll->tbf_poll.poll_tbf); if (!poll || poll->type !=PDCH_ULC_NODE_TBF_POLL || poll->tbf_poll.poll_tbf->direction != GPRS_RLCMAC_UL_TBF) LOGP(DL1IF, LOGL_DEBUG, "[%s] update TA = %u ignored due to " "unknown UL TBF on TRX = %d, TS = %d, FN = %d\n", p, ta, trx_no, ts, fn); - else { - tbf = as_ul_tbf(poll->tbf_poll.poll_tbf); + else if (ul_tbf) { /* we need to distinguish TA information provided by L1 * from PH-DATA-IND and PHY-RA-IND so that we can properly * update TA for given TBF diff --git a/src/gprs_ms.c b/src/gprs_ms.c index 0d6be4d..b9d130a 100644 --- a/src/gprs_ms.c +++ b/src/gprs_ms.c @@ -335,9 +335,13 @@ void ms_attach_tbf(struct GprsMs *ms, struct gprs_rlcmac_tbf *tbf) { - if (tbf_direction(tbf) == GPRS_RLCMAC_DL_TBF) + struct gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf); + struct gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf); + /* cannot be both DL and UL */ + OSMO_ASSERT(!(dl_tbf && ul_tbf)); + if (dl_tbf) ms_attach_dl_tbf(ms, as_dl_tbf(tbf)); - else + if (ul_tbf) ms_attach_ul_tbf(ms, as_ul_tbf(tbf)); } diff --git a/src/gprs_rlcmac_sched.cpp b/src/gprs_rlcmac_sched.cpp index 2adf1f3..b99ef9e 100644 --- a/src/gprs_rlcmac_sched.cpp +++ b/src/gprs_rlcmac_sched.cpp @@ -50,7 +50,8 @@ llist_for_each_entry(pos, &pdch->trx->ul_tbfs, list) { ul_tbf = as_ul_tbf((struct gprs_rlcmac_tbf *)pos->entry); - OSMO_ASSERT(ul_tbf); + if (!ul_tbf) + continue; /* this trx, this ts */ if (!ul_tbf->is_control_ts(pdch->ts_no)) continue; @@ -69,7 +70,8 @@ } llist_for_each_entry(pos, &pdch->trx->dl_tbfs, list) { dl_tbf = as_dl_tbf((struct gprs_rlcmac_tbf *)pos->entry); - OSMO_ASSERT(dl_tbf); + if (!dl_tbf) + continue; /* this trx, this ts */ if (!dl_tbf->is_control_ts(pdch->ts_no)) continue; @@ -459,6 +461,7 @@ "single block allocation at FN=%d\n", fn, block_nr, sba->fn); /* else, check uplink resource for polling */ } else if ((poll_tbf = pdch_ulc_get_tbf_poll(pdch->ulc, poll_fn))) { + struct gprs_rlcmac_ul_tbf *ul_tbf; LOGPDCH(pdch, DRLCMACSCHED, LOGL_DEBUG, "Received RTS for PDCH: FN=%d " "block_nr=%d scheduling free USF for polling at FN=%d of %s\n", fn, block_nr, poll_fn, tbf_name(poll_tbf)); @@ -466,8 +469,9 @@ * let's set its USF in the DL msg. This is not really needed, * but it helps understand better the flow when looking at * pcaps. */ - if (poll_tbf->direction == GPRS_RLCMAC_UL_TBF && as_ul_tbf(poll_tbf)->m_usf[ts] != USF_INVALID) - usf_tbf = as_ul_tbf(poll_tbf); + ul_tbf = as_ul_tbf(poll_tbf); + if (ul_tbf && poll_tbf->direction == GPRS_RLCMAC_UL_TBF && ul_tbf->m_usf[ts] != USF_INVALID) + usf_tbf = ul_tbf; /* else, search for uplink tbf */ } else if ((usf_tbf = sched_select_uplink(pdch, require_gprs_only))) { LOGPDCH(pdch, DRLCMACSCHED, LOGL_DEBUG, "Received RTS for PDCH: FN=%d " diff --git a/src/gprs_rlcmac_ts_alloc.cpp b/src/gprs_rlcmac_ts_alloc.cpp index 5ccbb9f..6566131 100644 --- a/src/gprs_rlcmac_ts_alloc.cpp +++ b/src/gprs_rlcmac_ts_alloc.cpp @@ -357,6 +357,8 @@ struct GprsMs *ms = tbf_->ms(); const gprs_rlcmac_tbf *tbf = tbf_; gprs_rlcmac_trx *trx = ms_current_trx(ms); + struct gprs_rlcmac_dl_tbf *dl_tbf; + struct gprs_rlcmac_dl_tbf *ul_tbf; LOGPAL(tbf, "A", single, use_trx, LOGL_DEBUG, "Alloc start\n"); @@ -406,12 +408,15 @@ /* The allocation will be successful, so the system state and tbf_/ms_ * may be modified from now on. */ - if (tbf->direction == GPRS_RLCMAC_UL_TBF) { - struct gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf_); + dl_tbf = as_dl_tbf(tbf_); + ul_tbf = as_ul_tbf(tbf_); + /* cannot be both DL and UL */ + OSMO_ASSERT(!(dl_tbf && ul_tbf)); + if (ul_tbf) { LOGPSL(tbf, LOGL_DEBUG, "Assign uplink TS=%d TFI=%d USF=%d\n", ts, tfi, usf); assign_uplink_tbf_usf(pdch, ul_tbf, tfi, usf); - } else { - struct gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf_); + } + if (dl_tbf) { LOGPSL(tbf, LOGL_DEBUG, "Assign downlink TS=%d TFI=%d\n", ts, tfi); assign_dlink_tbf(pdch, dl_tbf, tfi); } @@ -878,6 +883,8 @@ struct GprsMs *ms = tbf_->ms(); const gprs_rlcmac_tbf *tbf = tbf_; gprs_rlcmac_trx *trx; + struct gprs_rlcmac_dl_tbf *dl_tbf; + struct gprs_rlcmac_dl_tbf *ul_tbf; LOGPAL(tbf, "B", single, use_trx, LOGL_DEBUG, "Alloc start\n"); @@ -960,10 +967,14 @@ tbf_->first_common_ts = first_common_ts; tbf_->first_ts = first_ts; - if (tbf->direction == GPRS_RLCMAC_DL_TBF) - assign_dl_tbf_slots(as_dl_tbf(tbf_), trx, dl_slots, tfi); - else - assign_ul_tbf_slots(as_ul_tbf(tbf_), trx, ul_slots, tfi, usf); + dl_tbf = as_dl_tbf(tbf_); + ul_tbf = as_ul_tbf(tbf_); + /* cannot be both DL and UL */ + OSMO_ASSERT(!(dl_tbf && ul_tbf)); + if (dl_tbf) + assign_dl_tbf_slots(dl_tbf, trx, dl_slots, tfi); + if (ul_tbf) + assign_ul_tbf_slots(ul_tbf, trx, ul_slots, tfi, usf); bts_do_rate_ctr_inc(bts, CTR_TBF_ALLOC_ALGO_B); diff --git a/src/pdch.cpp b/src/pdch.cpp index 2ec40ce..f955444 100644 --- a/src/pdch.cpp +++ b/src/pdch.cpp @@ -452,7 +452,7 @@ return; } tbf = as_dl_tbf(poll->tbf_poll.poll_tbf); - if (tbf->tfi() != tfi) { + if (!tbf || tbf->tfi() != tfi) { LOGPTBFDL(tbf, LOGL_NOTICE, "PACKET DOWNLINK ACK with wrong TFI=%d, ignoring!\n", tfi); return; @@ -522,7 +522,7 @@ return; } tbf = as_dl_tbf(poll->tbf_poll.poll_tbf); - if (tbf->tfi() != tfi) { + if (!tbf || tbf->tfi() != tfi) { LOGPDCH(this, DRLCMAC, LOGL_NOTICE, "EGPRS PACKET DOWNLINK ACK with " "wrong TFI=%d, ignoring!\n", tfi); return; @@ -1063,8 +1063,8 @@ m_tbfs[tbf->direction][tbf->tfi()]->name()); m_num_tbfs[tbf->direction] += 1; - if (tbf->direction == GPRS_RLCMAC_UL_TBF) { - ul_tbf = as_ul_tbf(tbf); + ul_tbf = as_ul_tbf(tbf); + if (ul_tbf) { m_assigned_usf |= 1 << ul_tbf->m_usf[ts_no]; } m_assigned_tfi[tbf->direction] |= 1UL << tbf->tfi(); @@ -1083,8 +1083,8 @@ OSMO_ASSERT(m_num_tbfs[tbf->direction] > 0); m_num_tbfs[tbf->direction] -= 1; - if (tbf->direction == GPRS_RLCMAC_UL_TBF) { - ul_tbf = as_ul_tbf(tbf); + ul_tbf = as_ul_tbf(tbf); + if (ul_tbf) { m_assigned_usf &= ~(1 << ul_tbf->m_usf[ts_no]); } m_assigned_tfi[tbf->direction] &= ~(1UL << tbf->tfi()); diff --git a/src/tbf.cpp b/src/tbf.cpp index fcad879..e9e584f 100644 --- a/src/tbf.cpp +++ b/src/tbf.cpp @@ -270,15 +270,18 @@ void tbf_free(struct gprs_rlcmac_tbf *tbf) { /* update counters */ - if (tbf->direction == GPRS_RLCMAC_UL_TBF) { - gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf); + gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf); + gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(tbf); + /* cannot be both DL and UL */ + OSMO_ASSERT(!(dl_tbf && ul_tbf)); + if (ul_tbf) { bts_do_rate_ctr_inc(tbf->bts, CTR_TBF_UL_FREED); if (tbf->state_is(TBF_ST_FLOW)) bts_do_rate_ctr_inc(tbf->bts, CTR_TBF_UL_ABORTED); rate_ctr_group_free(ul_tbf->m_ul_egprs_ctrs); rate_ctr_group_free(ul_tbf->m_ul_gprs_ctrs); - } else { - gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf); + } + if (dl_tbf) { if (tbf->is_egprs_enabled()) { rate_ctr_group_free(dl_tbf->m_dl_egprs_ctrs); } else { @@ -291,9 +294,7 @@ /* Give final measurement report */ gprs_rlcmac_rssi_rep(tbf); - if (tbf->direction == GPRS_RLCMAC_DL_TBF) { - gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(tbf); - + if (dl_tbf) { dl_tbf->abort(); dl_tbf->cleanup(); } @@ -623,7 +624,10 @@ void gprs_rlcmac_tbf::poll_timeout(struct gprs_rlcmac_pdch *pdch, uint32_t poll_fn, enum pdch_ulc_tbf_poll_reason reason) { uint16_t pgroup; + gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this); gprs_rlcmac_ul_tbf *ul_tbf = as_ul_tbf(this); + /* cannot be both DL and UL */ + OSMO_ASSERT(!(dl_tbf && ul_tbf)); LOGPTBF(this, LOGL_NOTICE, "poll timeout for FN=%d, TS=%d (curr FN %d)\n", poll_fn, pdch->ts_no, bts_current_frame_number(bts)); @@ -690,9 +694,7 @@ /* Timeout waiting for CTRL ACK acking Pkt Cell Change Continue */ osmo_fsm_inst_dispatch(m_ms->nacc->fi, NACC_EV_TIMEOUT_CELL_CHG_CONTINUE, NULL); return; - } else if (direction == GPRS_RLCMAC_DL_TBF) { - gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this); - + } else if (dl_tbf) { if (!(dl_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK))) { LOGPTBF(this, LOGL_NOTICE, "Timeout for polling PACKET DOWNLINK ACK: %s\n", @@ -784,6 +786,7 @@ void gprs_rlcmac_tbf::handle_timeout() { int current_fn = bts_current_frame_number(bts); + gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this); LOGPTBF(this, LOGL_DEBUG, "timer 0 expired. cur_fn=%d\n", current_fn); @@ -798,8 +801,7 @@ } /* Finish waiting after IMM.ASS confirm timer for CCCH assignment (see timer X2002) */ - if ((state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))) { - gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this); + if (dl_tbf && (state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))) { dl_tbf->m_wait_confirm = 0; if (dl_tbf->state_is(TBF_ST_ASSIGN)) { tbf_assign_control_ts(dl_tbf); -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/25182 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I8ce21be6836549b47a606c00b793d6f005964c5c Gerrit-Change-Number: 25182 Gerrit-PatchSet: 1 Gerrit-Owner: neels <nhofmeyr at sysmocom.de> Gerrit-MessageType: newchange -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210809/2a652101/attachment.htm>