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
diff --git a/src/bts.cpp b/src/bts.cpp
index a91fe8d..b5fdfee 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -1162,22 +1162,21 @@
{
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
*/
if (is_rach)
- set_tbf_ta(tbf, (uint8_t)ta);
+ set_tbf_ta(ul_tbf, (uint8_t)ta);
else
- update_tbf_ta(tbf, ta);
+ update_tbf_ta(ul_tbf, ta);
}
}
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..88dc024 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_ul_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_ul_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);