pdch: Fix heap-use-after-free in pdch->ulc

In existing previous code, pdch->ulc would be freed in
gprs_rlcmac_pdch::free_resources() when  it became disabled as per PCUIF
info_ind (for instance, when a DYN TS is switched PDCH->SDCCH8).
However, pdch->ulc was so far only allocated during pdch_init, which is
only called during bts_alloc() time.
Hence, after first info_ind disabling it, if it became again enabled
(again by info_ind re-enabling it after SDCCH8 was not longer in use),
the pdch->ulc would be used again but it would point to freed memory.

Let's rearrange how/when resources are freed to make it more logical.
With this patch, pdch internal resources are freed upon ->disable(), and
re-allocated upon ->enable().

Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7
This commit is contained in:
Pau Espin 2021-06-30 16:03:06 +02:00
parent 1989a19066
commit 4f67a9bf46
6 changed files with 36 additions and 19 deletions

View File

@ -121,7 +121,8 @@ static void pcu_sock_close(int lost)
}
#endif
for (ts = 0; ts < 8; ts++)
pdch_disable(&bts->trx[trx].pdch[ts]);
if (pdch_is_enabled(&bts->trx[trx].pdch[ts]))
pdch_disable(&bts->trx[trx].pdch[ts]);
/* FIXME: NOT ALL RESOURCES are freed in this case... inconsistent with the other code. Share the code with pcu_l1if.c
for the reset. */
bts_trx_free_all_tbf(&bts->trx[trx]);

View File

@ -650,7 +650,7 @@ bssgp_failed:
for (trx_nr = 0; trx_nr < ARRAY_SIZE(bts->trx); trx_nr++) {
bts->trx[trx_nr].arfcn = info_ind->trx[trx_nr].arfcn;
for (ts_nr = 0; ts_nr < ARRAY_SIZE(bts->trx[0].pdch); ts_nr++)
bts->trx[trx_nr].pdch[ts_nr].free_resources();
bts->trx[trx_nr].pdch[ts_nr].disable();
}
gprs_bssgp_destroy(bts);
exit(0);
@ -818,7 +818,6 @@ bssgp_failed:
} else {
if (pdch->is_enabled()) {
pcu_tx_act_req(bts, pdch, 0);
pdch->free_resources();
pdch->disable();
}
}

View File

@ -139,19 +139,24 @@ void pdch_init(struct gprs_rlcmac_pdch *pdch, struct gprs_rlcmac_trx *trx, uint8
/* Initialize the PTCCH/D message (Packet Timing Advance Control Channel) */
memset(pdch->ptcch_msg, PTCCH_TAI_FREE, PTCCH_TAI_NUM);
memset(pdch->ptcch_msg + PTCCH_TAI_NUM, PTCCH_PADDING, 7);
pdch->ulc = pdch_ulc_alloc(pdch, trx->bts);
}
void gprs_rlcmac_pdch::enable()
{
/* TODO: Check if there are still allocated resources.. */
OSMO_ASSERT(m_is_enabled == 0);
INIT_LLIST_HEAD(&paging_list);
OSMO_ASSERT(!this->ulc);
this->ulc = pdch_ulc_alloc(this, trx->bts);
m_is_enabled = 1;
}
void gprs_rlcmac_pdch::disable()
{
/* TODO.. kick free_resources once we know the TRX/TS we are on */
OSMO_ASSERT(m_is_enabled == 1);
this->free_resources();
m_is_enabled = 0;
}
@ -159,10 +164,6 @@ void gprs_rlcmac_pdch::free_resources()
{
struct gprs_rlcmac_paging *pag;
/* we are not enabled. there should be no resources */
if (!is_enabled())
return;
/* kick all TBF on slot */
pdch_free_all_tbf(this);
@ -171,6 +172,7 @@ void gprs_rlcmac_pdch::free_resources()
talloc_free(pag);
talloc_free(this->ulc);
this->ulc = NULL;
}
struct gprs_rlcmac_paging *gprs_rlcmac_pdch::dequeue_paging()
@ -1174,6 +1176,12 @@ void pdch_free_all_tbf(struct gprs_rlcmac_pdch *pdch)
}
}
void pdch_disable(struct gprs_rlcmac_pdch *pdch) {
void pdch_disable(struct gprs_rlcmac_pdch *pdch)
{
pdch->disable();
}
bool pdch_is_enabled(const struct gprs_rlcmac_pdch *pdch)
{
return pdch->is_enabled();
}

View File

@ -55,8 +55,6 @@ struct gprs_rlcmac_pdch {
bool add_paging(uint8_t chan_needed, const struct osmo_mobile_identity *mi);
void free_resources();
bool is_enabled() const;
void enable();
@ -145,6 +143,8 @@ private:
enum gprs_rlcmac_tbf_direction dir);
gprs_rlcmac_tbf *tbf_by_tfi(uint8_t tfi,
enum gprs_rlcmac_tbf_direction dir);
void free_resources();
#endif
uint8_t m_num_tbfs[2];
@ -191,6 +191,7 @@ extern "C" {
void pdch_init(struct gprs_rlcmac_pdch *pdch, struct gprs_rlcmac_trx *trx, uint8_t ts_nr);
void pdch_free_all_tbf(struct gprs_rlcmac_pdch *pdch);
void pdch_disable(struct gprs_rlcmac_pdch *pdch);
bool pdch_is_enabled(const struct gprs_rlcmac_pdch *pdch);
#ifdef __cplusplus
}
#endif

View File

@ -2282,7 +2282,7 @@ static void test_tbf_ws()
the_pcu->alloc_algorithm = alloc_algorithm_b;
bts->trx[0].pdch[2].enable();
bts->trx[0].pdch[3].enable();
bts->trx[0].pdch[4].enable();
/* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts() */
bts->trx[0].pdch[5].enable();
gprs_bssgp_init(bts, 4234, 4234, 1, 1, false, 0, 0, 0);
@ -2327,7 +2327,7 @@ static void test_tbf_update_ws(void)
the_pcu->alloc_algorithm = alloc_algorithm_b;
bts->trx[0].pdch[2].enable();
bts->trx[0].pdch[3].enable();
bts->trx[0].pdch[4].enable();
/* bts->trx[0].pdch[4].enable(); Already enabled during setup_bts()) */
bts->trx[0].pdch[5].enable();
gprs_bssgp_init(bts, 5234, 5234, 1, 1, false, 0, 0, 0);

View File

@ -49,11 +49,19 @@ static void print_ulc_nodes(struct pdch_ulc *ulc)
}
}
static struct gprs_rlcmac_bts *setup_new_bts(void)
{
struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
pdch->enable();
return bts;
}
static void test_reserve_multiple()
{
printf("=== start: %s ===\n", __FUNCTION__);
const uint32_t fn = 20;
struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
struct gprs_rlcmac_bts *bts = setup_new_bts();
struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
struct gprs_rlcmac_tbf *tbf1 = (struct gprs_rlcmac_tbf*)0x1234; /*Dummy pointer */
struct gprs_rlcmac_tbf *tbf2 = (struct gprs_rlcmac_tbf*)0x5678; /*Dummy pointer */
@ -172,7 +180,7 @@ static void test_fn_wrap_around()
the_pcu->alloc_algorithm = _alloc_algorithm_dummy;
struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
struct gprs_rlcmac_bts *bts = setup_new_bts();
struct GprsMs *ms = ms_alloc(bts, 0x12345678);
struct gprs_rlcmac_tbf *tbf1 = tbf_alloc_dl_tbf(bts, ms, 0, true);
struct gprs_rlcmac_pdch *pdch = &tbf1->trx->pdch[0];
@ -215,7 +223,7 @@ static void test_fn_wrap_around()
static void test_next_free_fn_sba()
{
printf("=== start: %s ===\n", __FUNCTION__);
struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
struct gprs_rlcmac_bts *bts = setup_new_bts();
struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
struct gprs_rlcmac_sba *sba1, *sba2, *sba3, *sba4;
@ -239,7 +247,7 @@ static void test_next_free_fn_sba()
static void test_next_free_fn_rrbp()
{
printf("=== start: %s ===\n", __FUNCTION__);
struct gprs_rlcmac_bts *bts = bts_alloc(the_pcu, 0);
struct gprs_rlcmac_bts *bts = setup_new_bts();
struct gprs_rlcmac_pdch *pdch = &bts->trx[0].pdch[0];
struct gprs_rlcmac_sba *sba1;
uint32_t poll_fn, curr_fn;