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/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-pcu/+/24812 ) Change subject: pdch: Fix heap-use-after-free in pdch->ulc ...................................................................... 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 --- M src/pcu_l1_if.cpp M src/pdch.cpp M src/pdch.h M tests/tbf/TbfTest.cpp M tests/ulc/PdchUlcTest.cpp 5 files changed, 26 insertions(+), 17 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/12/24812/1 diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp index 2373f60..5aa8849 100644 --- a/src/pcu_l1_if.cpp +++ b/src/pcu_l1_if.cpp @@ -650,7 +650,7 @@ 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 @@ } else { if (pdch->is_enabled()) { pcu_tx_act_req(bts, pdch, 0); - pdch->free_resources(); pdch->disable(); } } diff --git a/src/pdch.cpp b/src/pdch.cpp index 9321384..fbfc474 100644 --- a/src/pdch.cpp +++ b/src/pdch.cpp @@ -139,19 +139,24 @@ /* 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 @@ { 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 @@ talloc_free(pag); talloc_free(this->ulc); + this->ulc = NULL; } struct gprs_rlcmac_paging *gprs_rlcmac_pdch::dequeue_paging() diff --git a/src/pdch.h b/src/pdch.h index cfa0195..b8f2911 100644 --- a/src/pdch.h +++ b/src/pdch.h @@ -55,8 +55,6 @@ 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 @@ 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]; diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp index 2c2c7ae..5065e90 100644 --- a/tests/tbf/TbfTest.cpp +++ b/tests/tbf/TbfTest.cpp @@ -2282,7 +2282,7 @@ 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 @@ 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); diff --git a/tests/ulc/PdchUlcTest.cpp b/tests/ulc/PdchUlcTest.cpp index c980e33..3547bf6 100644 --- a/tests/ulc/PdchUlcTest.cpp +++ b/tests/ulc/PdchUlcTest.cpp @@ -49,11 +49,19 @@ } } +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 @@ 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_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_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; -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/24812 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: Id51f5f6a54ac9f24b784c17bc360ac38f5726fc7 Gerrit-Change-Number: 24812 Gerrit-PatchSet: 1 Gerrit-Owner: pespin <pespin at sysmocom.de> Gerrit-MessageType: newchange -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210630/bd6886cf/attachment.htm>